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

Handling of double quotes in command arguments #182

Open
aayla-secura opened this issue Oct 4, 2022 · 9 comments
Open

Handling of double quotes in command arguments #182

aayla-secura opened this issue Oct 4, 2022 · 9 comments

Comments

@aayla-secura
Copy link

aayla-secura commented Oct 4, 2022

As far as I understand from the manual, double quotes can be used to escape special characters in any context and they should always be balanced (or escaped with a \). When it comes to command-line arguments though, I observe the following behaviour (on version 1.9.9 and prior; apologies I'm not able to test on the latest version):

The following entry:

   test ALL = NOPASSWD: /bin/ls " foo"

results in user test being able to run these two commands:

   sudo ls '" foo"'
   sudo ls '"' 'foo"'

but not what is expected:

   sudo ls ' foo'

and the following entry:

   test ALL = NOPASSWD: /bin/ls ""

results in user test being able to run both:

   sudo ls '""'
   sudo ls
@millert
Copy link
Collaborator

millert commented Oct 4, 2022

Double quotes can be used to escape special characters in certain cases, but not command line arguments. The use of "" in command line arguments is a special case that means no command line arguments are allowed. I've verified that this works as expected:

testuser ALL= /bin/ls ""

$ sudo ls
[succeeds]
$ sudo ls foo
Sorry, user testuser is not allowed to execute '/bin/ls foo' as root on hostname

@aayla-secura
Copy link
Author

aayla-secura commented Oct 4, 2022

Double quotes can be used to escape special characters in certain cases, but not command line arguments. The use of "" in command line arguments is a special case that means no command line arguments are allowed. I've verified that this works as expected:

testuser ALL= /bin/ls ""

$ sudo ls
[succeeds]
$ sudo ls foo
Sorry, user testuser is not allowed to execute '/bin/ls foo' as root on hostname

Firstly, I think this is not stated clearly in the man page. Secondly using "", while it allows no arguments yes, it also allows a single argument of '""' so that's definitely a bug. Thirdly, why does a single entry /bin/ls " foo" allow two different versions of arguments to be passed: a single argument of '" foo"' and two arguments of '"' and 'foo"'? It makes no sense, no administrator would even think that's the case and makes it impossible to restrict command arguments properly.

@millert
Copy link
Collaborator

millert commented Oct 4, 2022

Your shell is interpreting those quotes, they are not passed verbatim to sudo. However, I don't see the behavior you are describing:

testuser ALL= /bin/ls ""

$ sudo ls
[succeeds]
$ sudo ls ""
Sorry, user testuser is not allowed to execute '/bin/ls ' as root on hostname.

@aayla-secura
Copy link
Author

aayla-secura commented Oct 4, 2022

Your shell is interpreting those quotes, they are not passed verbatim to sudo.

That's incorrect. Of course they are passed verbatim to sudo. I've added the single quotes to show how I am passing the arguments to the shell. The shell understands the single quote (') and passes the rest to sudo. What you've written sudo ls "" means sudo ls '' not sudo ls '""'.

sudoers:

test ALL = NOPASSWD: /usr/bin/ls ""
test ALL = NOPASSWD: /usr/bin/stat " foo"

Results in:

test@ip-172-31-44-38:~$ sudo -l
Matching Defaults entries for test on ip-172-31-44-38:
    env_reset, mail_badpass, secure_path=/usr/local/sbin\:/usr/local/bin\:/usr/sbin\:/usr/bin\:/sbin\:/bin\:/snap/bin, use_pty

User test may run the following commands on ip-172-31-44-38:
    (root) NOPASSWD: /usr/bin/ls \"\"
    (root) NOPASSWD: /usr/bin/stat \" foo\"
test@ip-172-31-44-38:~$ sudo ls '""'
'""'
test@ip-172-31-44-38:~$ sudo ls
' foo'  '"'  '" foo"'  '""'  "' foo'"  'foo"'
test@ip-172-31-44-38:~$ sudo stat '" foo"'
  File: " foo"
  Size: 0               Blocks: 0          IO Block: 4096   regular empty file
Device: ca01h/51713d    Inode: 516100      Links: 1
Access: (0664/-rw-rw-r--)  Uid: ( 1001/    test)   Gid: ( 1001/    test)
Access: 2022-10-04 18:20:28.687560907 +0000
Modify: 2022-10-04 18:20:28.687560907 +0000
Change: 2022-10-04 18:20:28.687560907 +0000
 Birth: 2022-10-04 18:20:28.687560907 +0000
test@ip-172-31-44-38:~$ sudo stat '"' 'foo"'
  File: "
  Size: 0               Blocks: 0          IO Block: 4096   regular empty file
Device: ca01h/51713d    Inode: 516098      Links: 1
Access: (0664/-rw-rw-r--)  Uid: ( 1001/    test)   Gid: ( 1001/    test)
Access: 2022-10-04 18:20:28.687560907 +0000
Modify: 2022-10-04 18:20:28.687560907 +0000
Change: 2022-10-04 18:20:28.687560907 +0000
 Birth: 2022-10-04 18:20:28.687560907 +0000
  File: foo"
  Size: 0               Blocks: 0          IO Block: 4096   regular empty file
Device: ca01h/51713d    Inode: 516099      Links: 1
Access: (0664/-rw-rw-r--)  Uid: ( 1001/    test)   Gid: ( 1001/    test)
Access: 2022-10-04 18:20:28.687560907 +0000
Modify: 2022-10-04 18:20:28.687560907 +0000
Change: 2022-10-04 18:20:28.687560907 +0000
 Birth: 2022-10-04 18:20:28.687560907 +0000

@millert
Copy link
Collaborator

millert commented Oct 4, 2022

Sorry, you are correct, I didn't notice that you have the "" in single quotes. That is indeed a bug.

@aayla-secura
Copy link
Author

Sorry, you are correct, I didn't notice that you have the "" in single quotes. That is indeed a bug.

Thanks for acknowledging. And I had initially forgotten the ' around "" (edited later), I only had them in the second case of '" foo"', '"' and ' foo"', so no worries.

millert added a commit that referenced this issue Oct 4, 2022
If the empty string is specified in sudoers, no user args are allowed.
GitHub issue #182.
@aayla-secura
Copy link
Author

Actually, I realise this issue is not just about double quotes, but spaces too. I.e.

test ALL = NOPASSWD: /bin/ls foo bar

Would allow both sudo ls 'foo bar' and sudo ls foo bar.

@millert
Copy link
Collaborator

millert commented Oct 4, 2022

That's a different problem. Sudo concatenates all the command line arguments into a single string before matching so there is no way for it to tell the difference. The matching of command line arguments is far from ideal but fixing it by, say, requiring double quotes for command line arguments that contain whitespace in sudoers risks breaking existing configurations.

This at least is documented in sudoers, which says "Command line arguments are matched as a single, concatenated string."

@aayla-secura
Copy link
Author

I see. Thanks for pointing out where in the manual it says that, I had missed it. My thoughts on this is that it's worth changing how command-line arguments are matched in order to allow distinguishing e.g. 'foo bar' (or foo\ bar) from foo bar. I can think of edge cases where the current way of parsing would lead to bypasses with security implications.

millert added a commit that referenced this issue Oct 5, 2022
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

No branches or pull requests

2 participants