Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Settings PIN code protection. #2565

Closed
wants to merge 2 commits into from
Closed

Settings PIN code protection. #2565

wants to merge 2 commits into from

Conversation

blazoncek
Copy link
Collaborator

Protect settings pages with PIN code.

@@ -46,6 +33,12 @@
fO.value = '';
return false;
}
function checkNum(o) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why only numbers? While PIN makes it pretty clear that numbers should normally be used, if we allow letters as well we don't need this function and the user can make slightly more secure PINs.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agreed that complexity is prefered but since we do not have HTTPS it makes no sense (well, less sense) to make pin ultra complex. 4 digit pin should provide adequate protection until we add HTTPS.

if (!s2[0]) strcpy_P(s2, PSTR("Redirecting..."));

if (!doReboot) serveMessage(request, 200, s, s2, (subPage == 1 || subPage == 6) ? 129 : 1);
if (subPage == 6) doReboot = true;
serveMessage(request, 200, s, s2, (subPage == 1 || (subPage == 6 && doReboot)) ? 129 : (correctPIN ? 1 : 10));
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Putting a longer redirect time here unfortunately won't defeat a script (or browser back button).
We need to lock the set.cpp code from accepting new PINs for a few seconds (same for OTA lock now that reboot on Save is removed)

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The actual brute force protection is in line 448.

@blazoncek
Copy link
Collaborator Author

Any progress on this?

@Aircoookie
Copy link
Member

Tested, looks good mostly :) Can merge after 0.13.0 release (how about tomorrow for 0.13.0?)

Two minor things I've noted:
You only get the /settings/pin page when going to a specific settings page (e. g. LED settings), but entering the correct pin redirects you back to the main settings menu. This is weird. I'd recommend we prompt for the PIN already for the main settings menu.

xml.cpp serves the cleartext pin to GetV(). Should do something similar to WiFi password instead (e.g. replace with asterisks)

@blazoncek
Copy link
Collaborator Author

Agreed on 0.13.

I also agree that returning to main setting page is annoying but couldn't figure out how to get the proper destination.
Also serving plaintext PIN looks awful. Unfortunately no good idea came to my mind how to prevent removing existing PIN if field is left blank.

@blazoncek blazoncek linked an issue Mar 28, 2022 that may be closed by this pull request
@blazoncek
Copy link
Collaborator Author

Is superseded by #2610

@blazoncek
Copy link
Collaborator Author

This has been superseded by #2737
Closing.

@blazoncek blazoncek closed this Aug 18, 2022
@blazoncek blazoncek deleted the settings-pin branch August 30, 2022 15:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Paasword for Config Tab
2 participants