kernelkit / infix

Linux :yellow_heart: NETCONF = Infix
https://kernelkit.org
GNU General Public License v2.0
50 stars 12 forks source link

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

Closed mattiaswal closed 1 month ago

mattiaswal commented 1 month ago

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)

wkz commented 1 month ago

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. :+1:

mattiaswal commented 1 month ago

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. :+1:

But why block bash at all?

wkz commented 1 month ago

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 commented 1 month ago

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 commented 1 month ago

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).