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

admin password is limited to 126 characters #411

Closed
dismantl opened this issue Aug 28, 2014 · 8 comments
Closed

admin password is limited to 126 characters #411

dismantl opened this issue Aug 28, 2014 · 8 comments
Assignees
Milestone

Comments

@dismantl
Copy link
Contributor

When setting a new admin password in the Basic Config menu, the password is trunctated to 126 characters. There should be no maximum password length.

@jheretic jheretic modified the milestone: 1.1.4 Sep 4, 2014
@gradyoti gradyoti self-assigned this Sep 24, 2014
@gradyoti
Copy link
Collaborator

Update:
Adding password string longer than 126 chars does not present an error to the user.
Passwords above 126 chars are rejected (old, valid password remains active).

@gradyoti
Copy link
Collaborator

Update #2:
It looks like this is actually a limit for the passwd utility on the nodes, which the setup wizard calls directly. Unfortunately, passwd does not pass any errors, it just truncates the password and accepts the first 127 chars. This means that setup wizard assumes the operation was successful, even though the full password was not accepted.

My suggestion is to validate the password field before calling passwd, and pass an error back to the user if the password is too long.

@gradyoti
Copy link
Collaborator

Update #3:
Added issue to commotion-router, tagged upstream:
opentechinstitute/commotion-router#146

@seamustuohy
Copy link
Collaborator

Thought we should add notes from the back and forth Grady and I had on this issue so that the exploration of this issue was captured for any future validation issues.

Questions about this bug.

Q: Is the password truncated in the text field? So that you can't add any more chars?
A: Because then we should be looking at the js running on the page and not at the back end.

Q: Is the password being rejected before the page is submitted to the back end?
A: Because then we are again exploring the Javascript

You can eliminate possibility of front-end issues by simply turning off javascript in your browser and testing if it is still rejected.

I didn't explore front end impact because I don't have a node to do the easy tests.

Q: Is an error issues to the user about a failed password entry when the password is above 126 chars?
A: Then we are looking at the back end code. (See below, that is what I explored a little bit.)

Well, I don't have a router to test here, but it looks like it is a limit by the command line passwd setter.

Here are the commands used to come to that conclusion, and none of the outputs.

NOTE: my current shell looks like this
([GIT-BRANCH]) [DEVICE NAME]:[FOLDER-NAME] $


(master) computer:luci-commotion $ git log -S "admin password"

(master) computer:luci-commotion $ git show --oneline
bdc1b72bab7e0fa7b2460d51d51c9a7b0c01926c

(master) computer:luci-commotion $ cat -n
luasrc/model/cbi/commotion/basic_ns.lua |grep -i admin

(master) WORKGROUP:luci-commotion $ cat -n
luasrc/model/cbi/commotion/basic_ns.lua |grep -A 40

(master) computer:luci-commotion $ cat -n
luasrc/model/cbi/commotion/basic_ns.lua |grep root_pass_check

(master) computer:luci-commotion $ cat -n
luasrc/model/cbi/commotion/basic_ns.lua |grep -A 40 143


***switched over to the luci repo here***

computer:luci-0.11 $ cat -n libs/sys/luasrc/sys.lua |grep -B 5 -A 40
setpasswd

While the older libraries (apps) use just views for user facing content and controllers for logic, all the interfaces that are in the setup-wizard process use the traditional structure of model (controls
data manipulation and logic), view (shows pretty), controller (just loads models). So when you are looking for the logic behind things that are done in setup wizard you should start with the controller to
identify the model using the url you are exploring, the model to identify the logic being used on the back-end, and any views that are explicitly loaded by the model.

The luci libraries /libs/web/luasrc/cbi.lua file has all the default views model load. You can look in there to see the views used by default by model objects and therefore the javascript that those views use on the page.

@dismantl
Copy link
Contributor Author

dismantl commented Oct 1, 2014

Thanks for the additional info, @elationfoundation. To answer the questions:

  1. The password is not truncated in the input field. You can put a password in there as long as you want.
  2. The password is not rejected by the javascript before submission.
  3. No error is returned by the back-end for overly long passwords.

Basically, you can input a password as long as you want, the LuCI page accepts the submission, and no errors are ever given, giving the user reason to think everything went fine. Yet the actual password stored by the system has been truncated silently.

@dismantl
Copy link
Contributor Author

dismantl commented Oct 1, 2014

My proposal to @gradyoti was to add a maxlength property to the input field on the front-end, and then do back-end validation, returning an error if the password is too long.

@gradyoti gradyoti mentioned this issue Oct 2, 2014
@gradyoti
Copy link
Collaborator

gradyoti commented Oct 2, 2014

Addressed by #438

@jheretic
Copy link
Member

jheretic commented Oct 6, 2014

Closed as per #438

@jheretic jheretic closed this as completed Oct 6, 2014
@jheretic jheretic removed the 3 - Done label Oct 7, 2014
@jheretic jheretic modified the milestones: 1.1.4, 1.1 Jan 8, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants