5 ms·

I admit that I read the code and completely overlooked the single equals sign. Makes me wonder why it would be so easy to change the userid. Shouldn’t there be

I admit that I read the code and completely overlooked the single equals sign. Makes me wonder why it would be so easy to change the userid. Shouldn’t there be some safeguards in place to stop the userid from being updated from unsafe places.

jdblair 6y ago
You make a good point, but in a monolithic kernel the kernel is the “safe place.” Most likely the effect of this would be subtle and not necessarily long lived.
jrockway 6y ago
Same! I saw the if statement, was 100% sure this was going to be an "= instead of ==" thing... and still missed it. I spent too much mental energy looking at ((__LOUD|__NOISES)) and missed the obvious "current_user = 'root'" statement.
cyphar 6y ago
These days it'd be harder to write code which is "easy to overlook" -- the innocent version would be something like if (/* ... */ || current_euid() == GLOBAL_ROOT_KUID) But the "backdoor" version would fail to compile (current_euid() is a macro but it's written to not be a permitted lvalue). You would need to write something more obvious like the following (and kernel devs would go "huh?" upon seeing the usage of current_cred() in this context) if (/* ... */ || current_cred()->euid = GLOBAL_ROOT_KUID) In addition, comparisons against UIDs directly are no longer as common because of user namespaces and capabilities -- correct code would be expected to look more like if (/* ... */ || capable(CAP_SYS_ADMIN)) Which you can't write as an "accidental" exploit. And since most permission checks these days use capabilities rather than raw UIDs you'd need to do commit_creds(get_cred(&init_cred)); Which is bound to raise more than a couple of eyebrows and is really non-trivial to hide (assuming you put it somewhere as obvious as this person did). But I will say that it would've been much more clever to hide it in a device driver which is widely included as a built-in in distribution kernels. I imagine if you managed to compromise Linus' machine (and there are ways of "hiding" changes in merge commits) then the best place would be to shove the change somewhere innocuous like the proc connector (which is reachable via unprivileged netlink, is enabled on most distribution kernels, and is not actively maintained so nobody will scream about it). But these days we also have bots which actively scan people's trees and try to find exploits (including the 0day project and syzkaller), so such obvious bugs probably would still be caught.
- perihelions 6y ago
  "(current_euid() is a macro but it's written to not be a permitted lvalue)" I'm not an expert at C. I followed up on this kernel macro out of curiosity, and it was a confusing learning experience because it turns out the forbidden assignment ({ x; }) = y; is silently permitted by GCC (for example, with -Wall --std={c99,c11,c18}), and does actually assign x=y. Even though that's expressly prohibited by the C standard (-Wpedantic). I assume this is old news to C programmers, but its insidiousness surprised me.
  - simias 6y ago
    Example #5984 of why I don't like the kernel's convention of masquerading macros as function by making them lowercase. I wasted so much time deciphering weird compile errors or strange behaviour only to finally realize that one of the function calls in the offending code was actually a macro in disguise. It's especially bad when some kernel macros, such as wait_event, don't even behave like a function would (evaluating the parameter repeatedly). One more thing Rust got right by suffixing macros with a mandatory !.
    - cyphar 6y ago
      The best one is "current" -- which is a macro that looks like a variable but becomes a function call and thus if you ever want to use the variable name "current" you will get build errors. :D
  - cyphar 6y ago
    Huh, I assumed (just as you did) that this would obviously not work -- but you're right that GCC ignores this and allows the assignment anyway. However it turns out that you still get a build error, and even the more explicit versions also give you a error: kernel/cred.c:763:17: error: assignment of member ‘euid’ in read-only object 763 | current_euid() = GLOBAL_ROOT_UID; | ^ kernel/cred.c:764:23: error: assignment of member ‘euid’ in read-only object 764 | current_cred()->euid = GLOBAL_ROOT_UID; | ^ kernel/cred.c:765:22: error: assignment of member ‘euid’ in read-only object 765 | current->cred->euid = GLOBAL_ROOT_UID; | ^ So it is blocked but not for the reason I thought. current_cred() returns a const pointer and all of the cred pointers in task_struct are also const. So you'd need to do something more like: ((struct cred *)current_cred())->euid = GLOBAL_ROOT_UID; Which is well beyond "eyebrow-raising" territory.
Cthulhu_ 6y ago
Same; my Java indoctrination is kicking in and is asking why that field is apparently public and there's no controls as to what process can set it. That said, counterpoint, it's the kernel and performance is super important; the overhead of adding setters (etc) or an utility function like "current->isRoot()" is probably a tradeoff they made at some point.
db48x 6y ago
Absolutely. This is an example of the poor design of the C language. Other languages that were around at the time C was created choose `:=` as assignment and `=` for equality tests, making this type of typo quite impossible. Common Lisp makes the Hamming distance even larger; equality tests are written as `(eq foo bar)`, while changing a value is `(setf foo bar)`. Common Lisp may have features which are undesirable in an OS kernel (garbage collection), but it does make the code wonderfully clear and easy to read.
- ed25519FUUU 6y ago
  := Still seems easy to overlook at a cursory glance.
  - kazinator 6y ago
    What db48x neglected to mention is that some of those languages also featured assignment as strictly a statement; it could not be a subexpression. As in: fun(x := 42); (* syntax error in Pascal *) x := 42; (* OK *) x = 42; (* hopefully a statement with no effect warning *) If assignment is a statement, it's possible to use the same token. Classic BASIC: 10 X = 5 20 IF X = 5 GOTO 10 This doesn't cause the C problem of mistaken assignment in place of a test, so it's rather ironic that C managed to shoot itself in the foot in spite of dedicating twice the number of tokens.
    - db48x 6y ago
      I'd forgotten that!
    - a1369209993 6y ago
      That is true, but has nothing to do with "==" vs "=" vs ":=". You can do: func(x = 42); /* syntax error: expression operator expected, got '=' */ x = 42; /* OK */ x == 42; /* warning: statement expression has no effect */ just fine if you require "=" to be a statement.

jdblair 6y ago

You make a good point, but in a monolithic kernel the kernel is the “safe place.” Most likely the effect of this would be subtle and not necessarily long lived.

jrockway 6y ago

Same! I saw the if statement, was 100% sure this was going to be an "= instead of ==" thing... and still missed it. I spent too much mental energy looking at ((__LOUD|__NOISES)) and missed the obvious "current_user = 'root'" statement.

cyphar 6y ago

These days it'd be harder to write code which is "easy to overlook" -- the innocent version would be something like if (/* ... */ || current_euid() == GLOBAL_ROOT_KUID) But the "backdoor" version would fail to compile (current_euid() is a macro but it's written to not be a permitted lvalue). You would need to write something more obvious like the following (and kernel devs would go "huh?" upon seeing the usage of current_cred() in this context) if (/* ... */ || current_cred()->euid = GLOBAL_ROOT_KUID) In addition, comparisons against UIDs directly are no longer as common because of user namespaces and capabilities -- correct code would be expected to look more like if (/* ... */ || capable(CAP_SYS_ADMIN)) Which you can't write as an "accidental" exploit. And since most permission checks these days use capabilities rather than raw UIDs you'd need to do commit_creds(get_cred(&init_cred)); Which is bound to raise more than a couple of eyebrows and is really non-trivial to hide (assuming you put it somewhere as obvious as this person did). But I will say that it would've been much more clever to hide it in a device driver which is widely included as a built-in in distribution kernels. I imagine if you managed to compromise Linus' machine (and there are ways of "hiding" changes in merge commits) then the best place would be to shove the change somewhere innocuous like the proc connector (which is reachable via unprivileged netlink, is enabled on most distribution kernels, and is not actively maintained so nobody will scream about it). But these days we also have bots which actively scan people's trees and try to find exploits (including the 0day project and syzkaller), so such obvious bugs probably would still be caught.

perihelions 6y ago
"(current_euid() is a macro but it's written to not be a permitted lvalue)" I'm not an expert at C. I followed up on this kernel macro out of curiosity, and it was a confusing learning experience because it turns out the forbidden assignment ({ x; }) = y; is silently permitted by GCC (for example, with -Wall --std={c99,c11,c18}), and does actually assign x=y. Even though that's expressly prohibited by the C standard (-Wpedantic). I assume this is old news to C programmers, but its insidiousness surprised me.
- simias 6y ago
  Example #5984 of why I don't like the kernel's convention of masquerading macros as function by making them lowercase. I wasted so much time deciphering weird compile errors or strange behaviour only to finally realize that one of the function calls in the offending code was actually a macro in disguise. It's especially bad when some kernel macros, such as wait_event, don't even behave like a function would (evaluating the parameter repeatedly). One more thing Rust got right by suffixing macros with a mandatory !.
  - cyphar 6y ago
    The best one is "current" -- which is a macro that looks like a variable but becomes a function call and thus if you ever want to use the variable name "current" you will get build errors. :D
- cyphar 6y ago
  Huh, I assumed (just as you did) that this would obviously not work -- but you're right that GCC ignores this and allows the assignment anyway. However it turns out that you still get a build error, and even the more explicit versions also give you a error: kernel/cred.c:763:17: error: assignment of member ‘euid’ in read-only object 763 | current_euid() = GLOBAL_ROOT_UID; | ^ kernel/cred.c:764:23: error: assignment of member ‘euid’ in read-only object 764 | current_cred()->euid = GLOBAL_ROOT_UID; | ^ kernel/cred.c:765:22: error: assignment of member ‘euid’ in read-only object 765 | current->cred->euid = GLOBAL_ROOT_UID; | ^ So it is blocked but not for the reason I thought. current_cred() returns a const pointer and all of the cred pointers in task_struct are also const. So you'd need to do something more like: ((struct cred *)current_cred())->euid = GLOBAL_ROOT_UID; Which is well beyond "eyebrow-raising" territory.

Cthulhu_ 6y ago

Same; my Java indoctrination is kicking in and is asking why that field is apparently public and there's no controls as to what process can set it. That said, counterpoint, it's the kernel and performance is super important; the overhead of adding setters (etc) or an utility function like "current->isRoot()" is probably a tradeoff they made at some point.

db48x 6y ago

Absolutely. This is an example of the poor design of the C language. Other languages that were around at the time C was created choose `:=` as assignment and `=` for equality tests, making this type of typo quite impossible. Common Lisp makes the Hamming distance even larger; equality tests are written as `(eq foo bar)`, while changing a value is `(setf foo bar)`. Common Lisp may have features which are undesirable in an OS kernel (garbage collection), but it does make the code wonderfully clear and easy to read.

ed25519FUUU 6y ago
:= Still seems easy to overlook at a cursory glance.
- kazinator 6y ago
  What db48x neglected to mention is that some of those languages also featured assignment as strictly a statement; it could not be a subexpression. As in: fun(x := 42); (* syntax error in Pascal *) x := 42; (* OK *) x = 42; (* hopefully a statement with no effect warning *) If assignment is a statement, it's possible to use the same token. Classic BASIC: 10 X = 5 20 IF X = 5 GOTO 10 This doesn't cause the C problem of mistaken assignment in place of a test, so it's rather ironic that C managed to shoot itself in the foot in spite of dedicating twice the number of tokens.
  - db48x 6y ago
    I'd forgotten that!
  - a1369209993 6y ago
    That is true, but has nothing to do with "==" vs "=" vs ":=". You can do: func(x = 42); /* syntax error: expression operator expected, got '=' */ x = 42; /* OK */ x == 42; /* warning: statement expression has no effect */ just fine if you require "=" to be a statement.