Hacker News Reader

Comment by rdtsc | Hacker News Reader

I'd like to hear how this was discovered and who's name ended up on git blame line next to that password.

I have done a malicious source code injection as part of a network security exercise at the university. That was before git or other sane source control and I basically inserted a semi-obfuscated piece of code in the source repository, which gave our team an advantage in the game (the game was to crack and find a weakness in a protocol, but the whole machine was a target/battlefield). The clever part was first rooting the server via suid vulnerability.

I won the contest, but while doing it I thought, yeah, this shit will never work in the real world. And then this story made me remember that. That's pretty crazy.

View on HN

"I'd like to hear how this was discovered and who's name ended up on git blame line next to that password."

Yes. We all want to know a lot more about exactly how that got into the source code. It may take forensics and detective work to find out, but there's probably enough log info to figure it out. Pressure needs to be kept on Juniper until they disclose this. They can't be allowed to get away with claiming "it just happened somehow."

So let's say it was the NSA who did it. You think they haven't already sent Juniper an NSL telling them to cease discussing this incident?

Personally, I'll be rather surprised if we hear any more about this from Juniper than we already have.

They have a lot to lose if they keep quiet -- customers, credibility, future business, etc.

What are the powers of NSL, can they be given to any entity / person / company for any reason at all and also gag them from discussion getting the NSL? What are the penalties for a company breaking the NSL silence? Jailing the CEO? NSA sending it would also reveal quite a lot and jeopardise their future operations, it seems they'd want to have plausable deniability of some sort.

Not sure what the penalties for disclosing an NSL are, but companies sometimes use warrant canaries[1] for this purpose.

Notably the one used by Apple disappeared some time last year.

[1]: https://en.wikipedia.org/wiki/Warrant_canary

Considering FBI is investigating, other one hand doesn't know what the other is doing, or at least if not more likely the other handticles of the U.S. (and other governments) have been depending on some versions of affected hardware for some time.

Don't assume that the FBI and NSA get along. There is constant infighting wherever intel/LE agencies overlap. The FBI is a couple steps down the pyramid from the NSA, and so would want to score a few points.

These are also vast organizations. Even if the NSA was behind this, there is a reasonable chance the official hierarchy doesn't know whether or not they were involved. This exploit could even be the result of a mistake, code never meant to be deployed.

There is inherent levels of deniability in the operations of such a code word clearance heavy organisation. It's entirely possible >75% have no idea the full capabilities and less than 5% know the impact any individual program has on the overall capability of the organisation.

That's how it should be if it runs like it is supposed to.

Compartmentalization is one of the basics of running a 3-letter agency.

I think new NSLs have been limited to 3 years now, but I doubt Juniper will ever say it was the NSA, or even that the backdoor that others used was because of an NSA algorithm (which would make both them and the NSA look bad). So in the end it will be more about Juniper's reluctance to say than it being gagged.

Companies that sell a lot to the US government have a tendency to make more "compromises" to protect the government, too, or do what the government says. See BlackBerry and its new found (or maybe older than we think) "anti-privacy"/pro-lawful intercept stance.

Doing that would be implicitly admitting they'd made America less safe, which might lead to them having less power.

If I was the NSA I'd blame those sneaky commies, because that might lead to me getting more power. I'd say "this Chinese attack wouldn't have gone undetected for so long if the NSA had access to Juniper's source code repos, and monitored on all their employees, to check for sleeper agents"

If you think about it a bit longer you'll realize it can not be a single name. Yes, someone wrote this code. But then someone else reviewed it and then a third person signed off on shipping the end result. It takes (or really, it should take) more than one person at a company like Juniper to get away with a trick like this. Unless their processes are completely broken, let's for the moment assume that they are not otherwise all bets are off. Juniper is going to be scratched from a lot of POs because of this story, if it turns out they don't have a process in place to protect against stuff like this and it does not involve two people or more operating in concert to circumvent that process then they're pretty much dead.

I don't think it's necessarily true, I remember Linus saying that for Linux' kernel they used a web of trust. At one point you had to trust some people, you can't watch every updates. And he said something along the line "if you're not doing cryptography/security like this, you're doing it wrong"

> At one point you had to trust some people, you can't watch every updates.

Yes, so Linus doesn't. But his web of trust is hierarchical. In case of some malicious code making its way into the kernel, there will be a chain of trusted people that can be made accountable for it.

If you're suggesting the reviewer wrote the patch you've eliminated the developer but you still have someone higher up in the chain that needs to be totally incompetent. Even so, that's still two people, not one.

Linus' web-of-trust is more like a pyramid of trust, he has a couple of lieutenants that he trusts but I highly doubt they in turn will blindly sign off on a commit on something as critical as this.

If I was doing an attack like this, I'd wait for an opportunity to slip it into a huge, mundane change.

For example, changing the repository from Mercurial to Git. Splitting or combining two repositories. Moving lots of files between directories. Running an autoformatter over the entire codebase. Something like that.

Every code review tool I've seen there's /some/ way you can get it to highlight thousands of trivial changes. And I don't know about you, but I can't promise I'd spot 1 evil change among 1000 trivial ones.

Yes, that would be the way to do it. And that is exactly why a change like that is just as dangerous as any other.

I did quite a bit of code review during a contract earlier this year. One day one of the programmers shows up with a 100K line changeset, a reformatting and clean up that he'd embarked on of his own accord. Imagine his surprise when I point blank refused the commit. He was extremely upset (understandably, it was a lot of work and well intended) but at the back of my mind were two things: one, we are already under pressure to get things to work and this does not add anything functional. two, if it does add anything functional it will be an error or a bug and there is no way I'm going to catch this manually reviewing all these changes (across 100's of files). So the safe thing to do is to simply not accept the change unless I can free up developer time to break this massive commit into smaller ones that can be reviewed and accepted one-by-one over a longer period of time. And that was a luxury we could not afford.

So too bad, one very inflexible reviewer and one very pissed off programmer. 1000 trivial changes in a single commit isn't going to fly (at least, not with me), unless I review each and every one of those with just as much attention as I would review a much smaller commit. It's that sort of attention to process details that probably makes me 'less than easy' (to put it mildly) to work with but I really feel that if a customer trusts me that I'm going to have to earn that and rubberstamping a change set because it is too large to review is definitely a breach of that trust.

We have a rule that you can only prettify (to defined style rules) when you are already making a change to that piece of code or file.

Otherwise it's hand off as a rule. It also stops programmers wasting time.

That's a great way to do it: fix-as-you-go. The same goes for adding unit tests where those are missing (in bulk, rather than as a set going with a recently touched piece of code), fixing up documentation issues and other global changes such as naming conventions and so on.

Hitting all of the code base in one go with a thing like that is asking for trouble.

Downvoters are cordially invited to state why they disagree with this.

I work on a team maintaining a large code base. We follow a similar rule concerning unit tests. Our testing over 5 years has slowly grown from 0% to give us ~75% coverage, by using the methodology you describe.

I'll offer another opinion on this - I an a frontend developer and so it's very common for me to have to deal with files that are written in PHP, HTML, JavaScript, and also sometimes including CSS embedded right in it.

Roughly speaking, PHP and JavaScript have a similar syntax and can be formatted/styled in a similar way, but HTML it a totally different beast and has very different structure, as well as CSS which is a glorified list of rules.

I don't refactor code for the sake of it, however, in an effort to read related files I often clean things up that I must read in order to edit another section. I don't discard my cleanups because I'm leaving it better than when I found it (and cleaning up a messy repository is tough, but a little in each commit can help).

So I agree that you should style where you're working - but there are valid reasons to refactor or restyle code in other places too. Here's an example of a file I've been updating lately:

- before: http://i.imgur.com/X37RFQD.png

- after: http://i.imgur.com/9cjiJxn.png

There are exceptions to every rule, and yours is a valid exception to the rule that in general one should not commit non-functional change-sets. If something is particularly ugly then you could clean it up but that change should not be taken for granted not to have any adverse effects, there is really no such thing as an 'innocent' change. I've seen whitespace changes break code far too frequently to not be suspicious of any change at all.

I believe that's the wrong way to do it. You can only prettify/reformat if you aren't making changes to the file. That way you review only the prettify changes and not the functional changes.

This will condition reviewers to push 'merge' rather than to actually review the code because the assumption will be that these are safe changes.

I agree with you, beautifying and reformatting code can mess up the diff, and all of a sudden, someone ends up with their name pointing to code they didn't right. Beautifying, reformatting should only done alone.

Agreed, refactoring and functional changes should certainly be in separate commits, and ideally in separate pull requests. jacquesm can still have her/his rule to only change code involved in the course of functional changes; it's just that the refactoring and functional-change PRs will come in close succession and the refactoring PR exists only because the functional-change PR does also.

every place i've ever worked would require the developer to notify or ask permission from the project lead before making a change of that scope, if for nothing else, just the time commitment.

That's how it should have gone. But to the persons defense, the codebase really was a mess and a cleanup would have really helped us in getting a better insight in what the heck was going on, it really was with the best of intentions. But code review tends to be a bottle-neck anyway and there is no such thing as an 'innocent change' in my book, especially not in code-bases that have too little automated test coverage and zero documentation.

You could use a tool to compare AST's for equality, at the least you could use it to highlight the real changes as opposed to whitespace cleanup, which would make this job much easier. Or perhaps compare binaries directly if you set up deterministic compilation.

Most repositories allow you to ignore whitespace changes if you want to.

For instance, github allows you to do a ?w=1 appended to a diff url to see the differences without seeing the whitespace differences (that's like git diff -w).

That's the correct choice, although it would have been nicer to review it (if possible).

That said, huge commits can be ok if they can (a) be reviewed very quickly, (b) be entirely generated by a script (review the script, and ensure that running it generates the proposed change), or (c) are small under e.g. git diff -w.

Incidentally, removing $SubversionThingy$ from the header of each file in a moderately-sized repository is enough to break the FishEye code reviewing tool...

You'd want one better -- slip it in when your co-worker was about to make a change, update the yet to be committed code in their local working directory. They'd have a low chance of noticing it but the blame would lie with them.

Remind me to never piss you off.

Hehe ;-) to beat a black hat, have to think like one.

Could have fooled me ;)

There certainly will be multiple people to blame for this at least because nobody noticed who should have been looking but, it could have been also been a single rogue/disgruntled employee as well. About to be laid off perhaps, or also playing on the black market selling 0-day exploits. Even better they managed to slip in the name of the co-worker they hate the most in the committer field.

I'll take that over a national intelligence operation: infiltrating, gaining trust, paying off multiple people (and thus risking revealing their hand and operations), I can see a single person, who found out what the price for 0-day exploits are and who was perhaps been unfairly treated, was about to be laid of or leave the country. This was their way of getting an extra bonus on the way out of the door.

That's one very good reason to always lock your screen when you go to the bathroom.

And another to review all commits of an employee that left on bad terms or that seems to think they've been treated unfairly.

You'd be surprised. While the -official- code will typically always get QA'd and reviewed, there are plenty of cases where custom code for a specific use case is done by one engineer and bypasses the proper channel because of deadlines.

Git does not give strong guarantees for authorship unless you are signing every single commit with GPG (which is only a recently available as of 1.7 or 1.8).

I assure you it will and that's been known since Myer's landmark paper on Subversion a long time ago. Interestingly, I predicted your move in a post earlier (last para):

https://news.ycombinator.com/item?id=10767504

Even university students come up with this approach. Status quo seems hopeless against nation states unless companies learn from the past and start doing high assurance at least where it counts. ;)

Yeah I remember it was an interesting thought process. I put so much energy into rooting the system, and after a short celebration when I got a root shell, it was a "well now what?" moment.

We debated intercepting the trafic to the game server and retransmitting (a man in the middle), initially. Then I wanted to run a hidden process (with some system looking like name) that have injected java byte code into the running server to give our team an advantage, but that proved too difficult, so the idea to just change the source in the repo that the TA kept on the same machine, was kind of a boring and un-exciting conclusion. It didn't seem very clever at all and felt like giving up.

Interesting case study. My background is high assurance security. Most attackers go for subversion there because it's powerful. You're the only one I've seen in a while to do it because it was easier. Often is if software source or config is available in a centralized place.

Btw, here's the Myer's paper in 1980 that fleshed out the threat and gave the field no excuse to have no seen much of it coming:

http://csrc.nist.gov/publications/history/myer80.pdf

Written in era of time-sharing machines but many (most?) lessons carry over.

Isn't the name on the relevant "git blame" (or equivalent) line irrelevant unless the team signs commits with GPG (or equivalent)? It's trivial to forge the author.

git blame was figurative, I don't even know if they use git, I basically meant the person who is responsible for injecting that line (if they ever find out). And as you said, yes, the first thing someone would do who would insert that is of course put someone else's name there.

"My God, it was Edward Snowden"

I think it's extremely, vanishingly unlikely that the code contains a line like this:

   if (strcmp(login_password, "<<< %s(un='%s') = %u") == 0) ...

This feels much more like a "reflections on trusting trust"-style toolchain compromise.

Someone crossed the border in a hurry ...

ScreenOS seems to predate git (the NetScreen line is an acquisition made in 2004, according to Wikipedia).

I'm not sure if the older VCSs were as tamper-proof as modern ones so the line may as well be part of the original commit which added password authentication, decade before the hack.

Reply on news.ycombinator.com