A critical security bug in tarsnap [LWN.net]

A critical security bug in tarsnap

Posted Jan 20, 2011 2:17 UTC (Thu) by wahern (subscriber, #37304)
In reply to: A critical security bug in tarsnap by nedbass
Parent article: A critical security bug in tarsnap

It uses OpenSSL for some things, but the API shown in the article does not come from OpenSSL. Glancing at crypto_aesctr.c, it appears that he's rolled his own counter mode. It seems he's using OpenSSL's AES implementation for encrypting the nonce and then XORing the block stream into his bytestream himself. This is, IIRC, exactly how counter mode works.

The weird thing is, though, the nonce isn't incremented in his crypto_aesctr_stream() (i.e. after each block generation, which is the natural way to do it). So I suspect that he's concocted some sort of protocol atop AES-CTR, which would explain why he didn't just use OpenSSL's API. Alternatively, maybe he wrote the code before OpenSSL provided AES-CTR. But AES-CTR has been in OpenSSL for several years, and it doesn't explain why the nonce isn't incremented in the obvious place.

The author's description of the issue doesn't answer these questions.

(Log in to post comments)

A critical security bug in tarsnap

Posted Jan 20, 2011 5:53 UTC (Thu) by wahern (subscriber, #37304) [Link]

*ugh*. This was bothering me so I investigated further. I see now what's happening. The code is indeed incrementing the counter in the lower half of the block. The nonce is for the upper half. tarsnap is using some sort of chunking scheme which resets the lower half of the counter between chunks. (It's not clear that such a scheme is necessary but I'll just presume it is.) The counter value is computed using the second approach specified by NIST recommendation SP800-38A Appendix B.2.

The code is indeed reimplementing OpenSSL's CTR support. There's no reason to do this unless it was implemented before OpenSSL exported that functionality. But regardless the bug isn't in the duplicated code; it's in code which would have been necessary whether using OpenSSL's CTR mode or not.

If there's a moral to this story, it's to properly comment code. All the existing comments just repeat in English _what_ the C code is doing (which is superfluous if you understand C well). None of the relevant comments explain _why_ the logic exists (i.e. explain the chunking scheme and the relation of the nonce to ensuring a unique counter). The why is the most important part, and had the why comment existed the bug might never had come about.

This incident is a classic lesson for how to properly comment code. Classic. They should use this as an example in classrooms.