As someone formerly involved fairly deeply with the Git ecosystem, this was a painful article to read. After the first item, I was like, “OK, any project can have a security bug. Sympathy for the GitHub Desktop team”. Then GCM, for which I was the product manager for a few years… “OK .NET made an assumption we didn’t connect to Git’s assumptions…” Then it just kept going 🤦 I was also technically the Git-LFS PM for a while (only for GitHub’s corporate interest in it, not for the OSS project).
The fact that these three independent projects had similar security issues DESPITE Git’s mitigations makes me think that maybe newline-terminated lines on stdout might just be a bad idea.
I think it’s just a failure in the decision between whitelisting vs blacklisting. Whenever possible, whitelist the inputs. We saw this the other day on the FizzBuzz post here on lobste.rs. They used types to whitelist the inputs such that most of the problems the interviewers threw at them were non-issues, because they whitelisted all the valid inputs very early on and solved tons and tons of problems before they could ever become bug reports.
Git instead opted to blacklist, which often ends up being the wrong decision, as we see here.
Eh, if you use any of the non-broken serialization formats, as suggested above, then you don’t need any allow/denylists.
As much as JSON is bad, it’s good enough to use as a lingua franca for tools due to its simplicity and pervasiveness.
I agree, but protocol changes, especially one’s with multiple implementations across code-bases is at the bare minimum a giant, non-technical, hard problem.
Carriage return, the malicious byte value…
Are there cases where it serves a useful purpose in contemporary computing? I only know of \r\n, where the \r is redundant (i.e. not useful).
Interesting that the new check for the carriage return gets a setting for overriding. I understand not wanting to break anyone’s setup, but my imagination is lacking for who would need it.
For these kinds of text-based protocols, it’s probably reasonable to reject any control character other than tab, newline and space (also no delete). And possibly complicate your code with recognizing and allowing \r\n.
As someone formerly involved fairly deeply with the Git ecosystem, this was a painful article to read. After the first item, I was like, “OK, any project can have a security bug. Sympathy for the GitHub Desktop team”. Then GCM, for which I was the product manager for a few years… “OK .NET made an assumption we didn’t connect to Git’s assumptions…” Then it just kept going 🤦 I was also technically the Git-LFS PM for a while (only for GitHub’s corporate interest in it, not for the OSS project).
The fact that these three independent projects had similar security issues DESPITE Git’s mitigations makes me think that maybe newline-terminated lines on stdout might just be a bad idea.
I think it’s just a failure in the decision between whitelisting vs blacklisting. Whenever possible, whitelist the inputs. We saw this the other day on the FizzBuzz post here on lobste.rs. They used types to whitelist the inputs such that most of the problems the interviewers threw at them were non-issues, because they whitelisted all the valid inputs very early on and solved tons and tons of problems before they could ever become bug reports.
Git instead opted to blacklist, which often ends up being the wrong decision, as we see here.
Eh, if you use any of the non-broken serialization formats, as suggested above, then you don’t need any allow/denylists.
As much as JSON is bad, it’s good enough to use as a lingua franca for tools due to its simplicity and pervasiveness.
I agree, but protocol changes, especially one’s with multiple implementations across code-bases is at the bare minimum a giant, non-technical, hard problem.
Carriage return, the malicious byte value… Are there cases where it serves a useful purpose in contemporary computing? I only know of \r\n, where the \r is redundant (i.e. not useful). Interesting that the new check for the carriage return gets a setting for overriding. I understand not wanting to break anyone’s setup, but my imagination is lacking for who would need it. For these kinds of text-based protocols, it’s probably reasonable to reject any control character other than tab, newline and space (also no delete). And possibly complicate your code with recognizing and allowing \r\n.
Terminals still interpret it as originally intended:
printf 'Now you see me\rNow you dont! \n'appears to print only the second message.The fact that software emulation of VT100-era terminals is still a part of contemporary computing is truly poetic – as in Vogon poetry.
And it goes even further: The video terminals were emulating teletypes - which had a real carriage to return.
And they don’t call it the ASCII character
BELfor nothing!could one take away from this:
when one can use ssh for auth, one should use ssh.
do not do custom parsers near security code
Passing auth down through shell commands is always an adhoc mess. Maybe instead pass down info on auth agent like ssh does with ssh agent?
Not a security person, are these fair takeaways?