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

Silent failure when selecting bash as login shell for non-admin user #616

Closed
mattiaswal opened this issue Sep 10, 2024 · 5 comments · Fixed by #629
Closed

Silent failure when selecting bash as login shell for non-admin user #616

mattiaswal opened this issue Sep 10, 2024 · 5 comments · Fixed by #629
Assignees
Labels
bug Something isn't working
Milestone

Comments

@mattiaswal
Copy link
Contributor

mattiaswal commented Sep 10, 2024

I am running in the test system, and just applies this operation:


  with test.step("Add new user 'newuser01' with password 'newuser01password'"):
        hashed_password = sha256_crypt.using(rounds=5000).hash(password)
        target.put_config_dict("ietf-system", {
            "system": {
                "authentication": {
                    "user": [
                        {
                            "name": username,
                            "password": hashed_password,
                            "shell": "infix-system:bash"
                        }
                    ]
                }
            }
        })

Show running:

 "ietf-system:system": {
    "hostname": "test-00-03-00",
    "authentication": {
      "user": [
        {
          "name": "admin",
          "password": "$factory$",
          "infix-system:shell": "bash"
        },
        {
          "name": "newuser01",
          "password": "$5$EjMp58600GXi7ZQp$kav3mH2PPmtnngaVHwnGWIkUIw5pXru8/j1cvGGvZf6",
          "infix-system:shell": "bash"
        }
      ]
    }
  },

but /etc/passwd:

sshd:x:104:105:SSH drop priv user:/var/empty:/bin/false
admin:x:1000:1000:Linux User,,,:/home/admin:/bin/bash
newuser01:x:1001:1001:Linux User,,,:/home/newuser01:/bin/false
admin@test-00-03-00:~$ 

This took me a while to figure out, then i look in the code and noticed it was only for admins. This validation should really move to YANG. Everything looked fine but it didn't work as expected.

Or why block /bin/bash for non-admin users at all, it should be up for the user to select it and take consequences for him (if any, each use-case is different)

@mattiaswal mattiaswal changed the title Not possible to configure new user with bash as shell Silent failure if selecting bash as login shell Sep 10, 2024
@mattiaswal mattiaswal changed the title Silent failure if selecting bash as login shell Silent failure if selecting bash as login shell for non admin user Sep 10, 2024
@wkz
Copy link
Contributor

wkz commented Sep 10, 2024

IIRC, it was done that way to just not have to worry about it until we knew the config was always NACM-validated (independent of the front-end used).

Now that that should be fixed, we can probably move that restriction from being hard-coded in confd; over to an NACM rule in the default config which accomplishes the same thing. 👍

@troglobit troglobit added bug Something isn't working triage Pending investigation & classification (CCB) labels Sep 11, 2024
@mattiaswal
Copy link
Contributor Author

IIRC, it was done that way to just not have to worry about it until we knew the config was always NACM-validated (independent of the front-end used).

Now that that should be fixed, we can probably move that restriction from being hard-coded in confd; over to an NACM rule in the default config which accomplishes the same thing. 👍

But why block bash at all?

@wkz
Copy link
Contributor

wkz commented Sep 11, 2024

Because I don't think we're at the point where we've thought about all the possible local privilege escalation vectors that are accessible from bash.

Therefore, I think it makes sense to have some extra guardrails in place. Like I said, you should ideally be able to override those rules, but I think the added resistance of having to modify the defaul NACM setup first, is good to give a hint to the administrator that this is maybe not the best move from a security perspective.

@mattiaswal
Copy link
Contributor Author

mattiaswal commented Sep 11, 2024

Because I don't think we're at the point where we've thought about all the possible local privilege escalation vectors that are accessible from bash.

Therefore, I think it makes sense to have some extra guardrails in place. Like I said, you should ideally be able to override those rules, but I think the added resistance of having to modify the defaul NACM setup first, is good to give a hint to the administrator that this is maybe not the best move from a security perspective.

So adding a new UNIX user that should be able to SSH and check something we not have implemented in the CLI should not be possible by default? I personally think that is a little bit strange. Since you first (as an admin) add that user, if you give the user shell bash, that is of course a risk, that is always the case. But for now you have to make that kind of user admin, isn't that a stranger behavior?

I do not see why you should not allow it, even since it may not be the best for security...

@troglobit
Copy link
Contributor

AFK discussion. Two possible ways forward:

  1. must expression, detect if in admin group, warn if user has bash/sh as shell
  2. expand NACM rule set to cover login shell setting (lock down ability for any user to set bash/sh for non-admin users)
  3. in change phase in ietf-system.c, check user's group and shell -> return fail

The third alternative seems like a simpler approach (for now).

@troglobit troglobit added this to the Infix v24.09 milestone Sep 11, 2024
@troglobit troglobit moved this to Todo in Infix & C:o Sep 11, 2024
@troglobit troglobit removed the triage Pending investigation & classification (CCB) label Sep 11, 2024
@troglobit troglobit moved this from Todo to In progress in Infix & C:o Sep 11, 2024
@mattiaswal mattiaswal changed the title Silent failure if selecting bash as login shell for non admin user Silent failure when selecting bash as login shell for non-admin user Sep 13, 2024
mattiaswal added a commit that referenced this issue Sep 13, 2024
Added some security information in YANG model, and it is now
up to the system administrator to handle potential security
issues.

This fix #616
@mattiaswal mattiaswal mentioned this issue Sep 13, 2024
13 tasks
mattiaswal added a commit that referenced this issue Sep 13, 2024
Added some security information in YANG model, and it is now
up to the system administrator to handle potential security
issues.

This fix #616
mattiaswal added a commit that referenced this issue Sep 13, 2024
Added some security information in YANG model, and it is now
up to the system administrator to handle potential security
issues.

This fix #616
mattiaswal added a commit that referenced this issue Sep 13, 2024
Added some security information in YANG model, and it is now
up to the system administrator to handle potential security
issues.

This fix #616
mattiaswal added a commit that referenced this issue Sep 13, 2024
Added some security information in YANG model, and it is now
up to the system administrator to handle potential security
issues.

This fix #616
mattiaswal added a commit that referenced this issue Sep 15, 2024
Added some security information in YANG model, and it is now
up to the system administrator to handle potential security
issues.

This fix #616
mattiaswal added a commit that referenced this issue Sep 16, 2024
Added some security information in YANG model, and it is now
up to the system administrator to handle potential security
issues.

This fix #616
mattiaswal added a commit that referenced this issue Sep 16, 2024
Added some security information in YANG model, and it is now
up to the system administrator to handle potential security
issues.

This fix #616
mattiaswal added a commit that referenced this issue Sep 16, 2024
Added some security information in YANG model, and it is now
up to the system administrator to handle potential security
issues.

This fix #616
mattiaswal added a commit that referenced this issue Sep 16, 2024
Added some security information in YANG model, and it is now
up to the system administrator to handle potential security
issues.

This fix #616
@github-project-automation github-project-automation bot moved this from In progress to Done in Infix & C:o Sep 17, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
Status: Done
Development

Successfully merging a pull request may close this issue.

3 participants