
Security Fix in Open BSD - wglb
http://code.bsd64.org/cvsweb/openbsd/src/sys/netinet/ip_esp.c.diff?r1=1.74;r2=1.75;f=h
======
tptacek
In 2001, Angelos Keromytis --- then a grad student at Penn, now a Columbia
professor --- added support for hardware-accelerated IPSEC NICs. When you have
an IPSEC NIC, the channel between the NIC and the IPSEC stack keeps state to
tell the stack not to bother doing the things the NIC already did, among them
validating the IPSEC ESP authenticator. Angelos' code had a bug; it appears to
have done the software check _only_ when the hardware had already done it, and
skipped it otherwise.

The bug happened during a change that simultaneously refactored and added a
feature to OpenBSD's ESP code; a comparison that should have been == was
instead !=; the "if" statement with the bug was originally and correctly !=,
but should have been flipped based on how the code was refactored.

HD Moore may as we speak be going through the pain of reconstituting a nearly
decade-old version of OpenBSD to verify the bug, but stipulate that it was
there, and here's what you get: IPSEC ESP packet authentication was disabled
if you didn't have hardware IPSEC. There is probably an elaborate man-in-the-
middle scenario in which this could get you traffic inspection, but it's
nowhere nearly as straightforward as leaking key bits.

To entertain the conspiracy theory, you're still suggesting that the FBI not
only introduced this bug, but also developed the technology required to MITM
ESP sessions, bouncing them through some secret FBI-developed middlebox.

One year later, Jason Wright from NETSEC (the company at the heart of the [I
think silly] allegations about OpenBSD IPSEC backdoors) fixed the bug.

It's interesting that the bug was fixed without an advisory (oh to be a fly on
the wall on ICB that day; Theo had a, um, a, "way" with his dev team). On the
other hand, we don't know what releases of OpenBSD actually had the bug right
now.

It seems vanishingly unlikely that there could have been anything deliberate
about this series of changes. You are unlikely to find anyone who will impugn
Angelos. Meanwhile, the diffs tell exactly the opposite of the story that Greg
Perry told.

~~~
caf
The most worrying thing about it is probably just that it took a year to
notice - you would think "throw a forged authenticator at it" would be in the
regression tests for an IPSEC implementation.

~~~
tptacek
"Regression tests for an IPSEC implementation". Heh.

~~~
irfn
lol.. are there any tests at all..

------
paperclip
I presume you are attempting to imply that this one of the 'leet' FBI
backdoors that the Perry email discussed. It would help if you actually said
why you thought this patch was interesting when submitting it to HN.

I hope we are not going to get a rash of inarticulate HN submissions for every
minor patch to openbsd which may have security implications.

I doubt this is in anyway related to the recent drama.

The initial bad patch which broke things looks like v1.63 of the file. This
was committed in 2001 by angelos.

Note: \- angelos has a greek email address (for what it is worth) \- 1.63 was
committed in 2001; the allegations of backdoors seem to be from 2000 and 1999
\- it appears to break things for all non-ipsec aware nics. This seems to
broad an error for an intentional backdoor

It also looks like the code didn't make it to a release branch before being
fixed - so if it was an attempted backdoor, it was not successful.

Disclaimer: I have no experience with BSD kernel code nor with IPSEC.

Edit: I initially thought that angelos also commited the fix as well. I think
I need to get my eyes tested

~~~
gcr

      > I hope we are not going to get a rash of inarticulate
      > HN submissions for every minor patch to openbsd which
      > may have security implications.
    

I do. Submissions are cheap and skippable. Plus, now I get to look at all
these small bugs and not make the same mistakes in my own code, which is both
a huge win in my book and something that one can't learn from a textbook.

~~~
paperclip
This submission prior to tptacek's (rather good) analysis was of little value.
The original submitter gave absolutely no context to the diff.

I don't think we can depend on tptacek always being on hand.

This bug itself is not novel and something any programmer (if they are being
honest) will admit to doing themselves.

The really interesting part of this story is not technical at all (and not
evident from the posted patch) - why did the openbsd team not feel it
necessary to release a security advisory for this bug. That decision may
tarnish their reputation more than any wild conspiracy claims.

~~~
tptacek
The submitter works about 15 away from me, for what it's worth.

------
gcr
Is this related to the alleged IPSEC backdoor? Either way, could we get some
context?

------
jey
Looking at the dates, this bug was fixed in under a month. Did the buggy code
really get released to production environments that fast? A priori unlikely
IMO, which means this "vulnerability" is instead just the normal development
process at work. I'm sure we've all committed code with mindbogglingly dumb
bugs at one point or another.

~~~
tptacek
A month? I'm getting over a year from my look; the bug was introduced with the
original feature in 2001.

~~~
jey
Oops, I'm an idiot. Obviously the linked page doesn't say when the original
buggy code was introduced; it only gives the last modification timestamp.

------
davidj
Fixing security holes without disclosing the problem to the public.. yup I've
seen that before from openbsd

------
Thangorodrim
Looking only at that diff, the fix is a break.

It inverts the sense of the test so that the authenticator of the packet is
used even if the tag is null.

~~~
tptacek
The "tag" is an mbuf tag. Mbuf tags aren't packet data; think of them like
per-packet session state, so that different layers of the stack (the network
drivers, any driver required to handle crypto goo done on the card, the
original IP code, and the IPSEC decapsulation and handling code) can talk to
each other without hand-coding a million disgusting callbacks throughout the
kernel.

In this code, "mtag" is the tag matching PACKET_TAG_IPSEC_IN_CRYPTO_DONE,
which the lower-level drivers use to tell the IPSEC code that the NIC already
did verification. If the tag _isn't_ there, then you want to do the
verification in software, which is what the fixed code does and the broken
code didn't do.

------
ctcherry
Could someone explain the significance of this change? What would this enable
or disable in practice?

~~~
calloc
As Thomas Ptacek more accurately describes in his post, this disables the ESP
Auth checks (after the fix) if the NIC has already completed them. Before the
fix, the software ESP auth check would run only when the NIC had already done
them, so never if the NIC had not done them.

------
tedunangst
Fix is 8 years old.

~~~
david_p
I believe this is a follow-up to this story : [http://marc.info/?l=openbsd-
tech&m=129236621626462&w...](http://marc.info/?l=openbsd-
tech&m=129236621626462&w=2)

I think the purpose of the link is to demonstrate some "suspicious" changes in
the IPSEC code.

