A critical security bug in tarsnap

*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.