61–70 of 144 posts
I think that this might be an typo or at least it has plausible deniability. I have changed my coding style to always put constant on left side just to avoid such an error (such typo gave me a few days of debugging multithreaded code and I have just said "Never again!!" :D)
Was this a backdoor or not? Following the comments on the article and previous posts here on HN it seems the jury is out AFAICS. The crucial question to me seems to be if this condition: options == (__WCLONE|__WALL) can be willfully introduced by a bad actor, and otherwise never really occur. Unfortunately I don't know this (not familiar with Linux development) but herein lies the answer it would seem.
Following the man pages: wait4's man page points to waitpid for details, and notes wait4 is deprecated in favor of waitpid. So see the linux notes of this: https://man7.org/linux/man-pages/man2/waitpid.2.html The following Linux-specific options [..] can also, since Linux 4.7, be used with waitid(): __WCLONE [...] This option is ignored if __WALL is also specified. __WALL So to trigger this: * You have to call a depr…
Has this happened since the source-control was changed to git? I imagine it would be almost impossible to break into Linus Torvald's git server amend previous commits, considering each one's hashed on the previous commits...
If you can break SHA1, that task would be easier.
That said, if you could rewrite an older commit, the change would only be applied in a fresh clone, right?
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.
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.
There should be safe guards against such errors. Even with approval, the reviewer may not notice it. Which brings up the question: how many more root-based backdoors are there now in the source code?
I wouldn't mind if languages simply mark assignments in conditions as errors. It's clever code, but clever code should be avoided in critical systems. And in general, I guess.
Won't gcc complain if you assign a variable within an if-statement?
Assignments in if-statement can be useful, and that's how you prevent the compiler from complaining. That warning is intended for honest mistakes, not to catch backdoors.
A uid of 0 being root is just such a bad idea to begin with because 0 is a default value of so many data types. It’s an accident waiting to happen and, in this case, a good way to hide something malicious as an accident.
>and, in this case, a good way to hide something malicious as an accident The number could've been 2342 and the backdoor would've worked exactly the same way.
Earlier quoted context omitted.
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 cur…
"(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…
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;
| ^
((struct cred *)current_cred())->euid = GLOBAL_ROOT_UID;
Earlier quoted context omitted.
If you can break SHA1, that task would be easier.
SHA1 is close to being broken, but it's not there yet, and Git will be migrating to a better algorithm. That said, if you could rewrite an older commit, the change would only be applied in a fresh clone, right?
I think so, assuming the fetch algorithm is using the hashes to get the deltas which I think it does.
I'm not sure about CVS but with GIT rewriting a _previous_ commit _object_ itself with different blobs but making the commit object itself have the _same_ hash by messing with it's comment wouldn't cause any difference in child commits since commits are pretty much independent other than the pointers to parent/child and incorporating that into it's hash (i.e they would have different trees so the changes would not propagate to the HEAD of the branch).
I think the only way have something end up in the HEAD of a branch AND persist is to break the SHA1 of a blob (i.e a file) by inserting the extra SHA1 breaking content into the blob itself rather than a commit tree (provided that exact blob hash is part of the tree in the HEAD of a branch). Then you would also need to hope that the malicious blob is fetched by the person who writes the next commit to be based upon the HEAD of that branch AND modifies the same file blob so that it persists into the next revision of the blob... seems pretty hard to pull off - pun intended
There is also the issue of pushing a blob that already exists on the remote according to the hash. Even with re-write permission GC might make that hard to do quickly.... I wonder if you would need direct access to the git server to do this.
[EDIT]
Thinking about swapping out SHA1 in the future, you would still want to rehash all of the blobs and trees to prevent SHA1 attacks on old blobs that are unchanged going forward to essentially prevent what I described above.
If you only hashed new blobs with the new algorithm you would need to wait until every file had been touched to be safe.