4 ms·

Yeah, I’m surprised that this patch is getting traction at all. If it’s a bug in the application, it should be fixed in the application. The kernel shouldn’t be

Yeah, I’m surprised that this patch is getting traction at all. If it’s a bug in the application, it should be fixed in the application. The kernel shouldn’t be changing the kernel-userspace interface to work around userspace bugs. There are plenty of bespoke binaries out there with no/lost source code that won’t get the benefit of scrutiny from the kernel dev team. I certainly wouldn’t assume that none of them invoke execve with a NULL or 0-item arg parameter, and merging this patch would break them without recourse.

jandrese 5y ago
Given that this behavior is already disallowed (and faulting) on BSD and at least some commercial Unix flavors there shouldn't be too much software out there that relies on it. I would lean towards making it an API error BSD style and taking breakage on a case by case basis. I'm not the person who would be called up however, so it's easy for me to say. Worst case you can create a sysctl variable to enable/disable the check for those corner cases where it can't be fixed and other precautions can be taken against the security vulnerability.
- josephcsible 5y ago
  > Given that this behavior is already disallowed (and faulting) on BSD and at least some commercial Unix flavors there shouldn't be too much software out there that relies on it. How much commercial software targets UNIX these days?
  - pjmlp 5y ago
    Enough to keep sales of AIX, Solaris, mainframes and several POSIX based RTOS going. And that little one named macOS as well.
    - CamJN 5y ago
      macOS allows argc == 0, it has a separate mechanism for providing the exec path of the executable to the process, namely the KERN_PROCARGS2 sysctl which includes the saved exec path along with argc, argv, envp, and the apple pointer. Amusingly enough, Apple has had a bug in their `ps` since 10.3 that assumes argc > 0, which actually results in a similar (but not likely harmful, despite ps being setuid) bug.
    - mst 5y ago
      If anything this breaks is already broken on macOS the argument probably becomes rather simpler, indeed.
kelnos 5y ago
Platform APIs should be designed in such a way that it is easy to use them securely. That's why we don't use strcpy() and friends anymore. (Yes, I know, not quite the same, as you can mitigate this with a new function and implore people not to use the old one.) Sure, the pkexec bug is now fixed, but I expect we'll see another similar one eventually. I know I personally have written a lot of C programs that assume argc>0 and argv!=NULL, and I'm sure there are thousands of such programs. I think returning EINVAL on exec in this case may be too much. I think the right move is probably for the kernel to just fix things up when exec is called in this way, so argv will always have at least one element, and that element 0 won't be NULL. I think any program that would break with that change is something I'd be able to live with.
- dmatech 5y ago
  It's very easy to use argc/argv securely as they currently are. POSIX clearly states that argc can be zero, but polkit didn't know that for some reason. When you're writing privileged software, you can't make assumptions about user input (and the specs) like this. I get that the vulnerability could have been prevented if Linux deviated from the spec like some of the BSDs might have, but we shouldn't make it the responsibility of kernel developers to make logic errors in user applications less likely.
  - mst 5y ago
    I agree that we shouldn't -make- it their responsibility but it's still worth asking in a case like this whether -this time- it's a better trade-off to do so anyway.
- saagarjha 5y ago
  > That's why we don't use strcpy() and friends anymore. We definitely do when safe to do so…
- mst 5y ago
  I am awful at C so this comment should be considered merely an anecdote but I was -so- sad when I (having started learning on BDSi and NetBSD) needed to port a program to other systems and discovered strlcpy wasn't there. (damnit, Jim, I'm already a segfault generator when I try to perpetrate C, I need all the help I can get!)
takeda 5y ago
Unlike other bugs, this change shouldn't break any application that is being called. argc >= 1 is the normal use case. So this would break anything that calls execve() with argc==0. Is such call providing any functionality? In addition to returning EINVAL being produced by not well written callers, another option would be to force argc==0 and fill up argv[0] with process name. That fix would make everything continue to work.
- yuubi 5y ago
  The ldd program that lists the shared libraries used by an executable at least used to exec with an empty argv, which the dynamic linker detected and listed libraries instead of linking and running. I learned that when I screwed up an inetd configuration and saw a list of libraries instead of the desired results. Edit: just tried on windows 10 WSL, and it had a sad (blue screen).
  - mst 5y ago
    WSL2 has made my life so much better that now I want to give it a hug. (also, TIL, thank you)

jandrese 5y ago

Given that this behavior is already disallowed (and faulting) on BSD and at least some commercial Unix flavors there shouldn't be too much software out there that relies on it. I would lean towards making it an API error BSD style and taking breakage on a case by case basis. I'm not the person who would be called up however, so it's easy for me to say. Worst case you can create a sysctl variable to enable/disable the check for those corner cases where it can't be fixed and other precautions can be taken against the security vulnerability.

josephcsible 5y ago
> Given that this behavior is already disallowed (and faulting) on BSD and at least some commercial Unix flavors there shouldn't be too much software out there that relies on it. How much commercial software targets UNIX these days?
- pjmlp 5y ago
  Enough to keep sales of AIX, Solaris, mainframes and several POSIX based RTOS going. And that little one named macOS as well.
  - CamJN 5y ago
    macOS allows argc == 0, it has a separate mechanism for providing the exec path of the executable to the process, namely the KERN_PROCARGS2 sysctl which includes the saved exec path along with argc, argv, envp, and the apple pointer. Amusingly enough, Apple has had a bug in their `ps` since 10.3 that assumes argc > 0, which actually results in a similar (but not likely harmful, despite ps being setuid) bug.
  - mst 5y ago
    If anything this breaks is already broken on macOS the argument probably becomes rather simpler, indeed.

kelnos 5y ago

Platform APIs should be designed in such a way that it is easy to use them securely. That's why we don't use strcpy() and friends anymore. (Yes, I know, not quite the same, as you can mitigate this with a new function and implore people not to use the old one.) Sure, the pkexec bug is now fixed, but I expect we'll see another similar one eventually. I know I personally have written a lot of C programs that assume argc>0 and argv!=NULL, and I'm sure there are thousands of such programs. I think returning EINVAL on exec in this case may be too much. I think the right move is probably for the kernel to just fix things up when exec is called in this way, so argv will always have at least one element, and that element 0 won't be NULL. I think any program that would break with that change is something I'd be able to live with.

dmatech 5y ago
It's very easy to use argc/argv securely as they currently are. POSIX clearly states that argc can be zero, but polkit didn't know that for some reason. When you're writing privileged software, you can't make assumptions about user input (and the specs) like this. I get that the vulnerability could have been prevented if Linux deviated from the spec like some of the BSDs might have, but we shouldn't make it the responsibility of kernel developers to make logic errors in user applications less likely.
- mst 5y ago
  I agree that we shouldn't -make- it their responsibility but it's still worth asking in a case like this whether -this time- it's a better trade-off to do so anyway.
saagarjha 5y ago
> That's why we don't use strcpy() and friends anymore. We definitely do when safe to do so…
mst 5y ago
I am awful at C so this comment should be considered merely an anecdote but I was -so- sad when I (having started learning on BDSi and NetBSD) needed to port a program to other systems and discovered strlcpy wasn't there. (damnit, Jim, I'm already a segfault generator when I try to perpetrate C, I need all the help I can get!)

takeda 5y ago

Unlike other bugs, this change shouldn't break any application that is being called. argc >= 1 is the normal use case. So this would break anything that calls execve() with argc==0. Is such call providing any functionality? In addition to returning EINVAL being produced by not well written callers, another option would be to force argc==0 and fill up argv[0] with process name. That fix would make everything continue to work.

yuubi 5y ago
The ldd program that lists the shared libraries used by an executable at least used to exec with an empty argv, which the dynamic linker detected and listed libraries instead of linking and running. I learned that when I screwed up an inetd configuration and saw a list of libraries instead of the desired results. Edit: just tried on windows 10 WSL, and it had a sad (blue screen).
- mst 5y ago
  WSL2 has made my life so much better that now I want to give it a hug. (also, TIL, thank you)