pycrypto is vulnerable and unmaintained

[lanodan]

See this thread : pycrypto/pycrypto#173

[tribut]

Is there a concrete reason to believe the "vulnerable" part is true? "unmaintained" seems accurate, though 😢

[lanodan]

Well… the vulnerability has been showed at the 32C3 (so nearly a year), confirmed by gentoo security team and probably other distros and myself. I don’t know if it’s useable in pure-python-otr.

Also I think using cryptography could be better instead of using pycryptodome. I’m not really a fan of having tons of “primitive” libraries which does the same thing, especially for crypto.

[koolfy]

Okay. First: the vulnerability is only precent in a portion of code that handles ECB block cipher mode. Nobody should use ECB anymore, and OTR sure doesn't. I had a look at the stream code (in a rush), and didn't see anything similar enough to make me insta-panic for now. The closest was https://github.com/dlitz/pycrypto/blob/7acba5f3a6ff10f1424c309d0d34d2b713233019/src/stream_template.c#L143 and it doesn't look dangerous to me. I have very very very little experience with buffer overflows, so this doesn't mean there isn't an exploit possible for this code else where, or even in the very line I just linked there. So from what I can personnally see, there is no cause for immediate alarm in the strict case of pure python OTR. Still, we should be woried that such vulnerabilities are present in our underlying code, that they are public but not fixed after 1y, and that vast portions of pycrypto (I think.) and more so pure python otr (I *know*.) haven't seen proper review. I'd say the first priority woud be for anyone with more experience with buffer overflows than me to investigate further if the code *we* call hasn't got any that you guys can see. The next priority would be to see what options do we have in regards to pycrypto's maintainment state. As much as I love cryptography, I cleary don't have time to take care of something like pycrypto. I'd love to help, but right now I'm not ever maintaining properly pure-python-otr... Basically pure-python-otr has someone who receives, reads, and reacts to github mails and pullrequests. From what I see pycrypto hasn't got even that. so... thoughts? ps: I'll be physically present at 33C3 in Hamburg to bring this topic up again and discuss it until we find a viable plan that doesn't consist on looking away and hoping for the best.

[koolfy]

Ok, so during 33C3 I rushed a very quick "hackathon" over this issue.
A few people showed up (at least two!), and some progress was made.

first, pycryptodome appears to be a very good option.
Here is my reasonning:
I drafted a quick list of criterias on top on my head, here it is:

• Is it actively maintained?
• Has it been built with security in mind?
• Does it implement all the primitives we need for OTRv3?
• How well is it documented?
• How easy is it to integrate to POTR? ("drop-in replacement"?)
• How easy is it to reach and communicate with maintainers?
• Have security reviews been conducted on its current form?
• Are there other projects actively using it and giving feedback?

So I ran this list on pycryptodome, feel free to do the same with other replacement candidates you want to suggest ;)

• It's actively maintained (several releases in 2016 and latest bugfix a few months ago --which is better than what I've been doing with potr ;) )
• Security focus extend to be defined (for example in terms of primitive implementation choices, the only ECC curve implemented is the NIST one :) )
• It does include all the primitives we need for OTRv3 (but will lack ECCs defined in the latest informal OTRv4 draft I've seen on otr-dev mailing list)
• Documentation looks fine.
• It's mostly a drop-in replacement, but not entirely. the counter handling for AES-CTR is not exactly how we'd need it to be. Currently the same problem applied to pycrypto, where potr implemented its own counter objects. More on this below.
• Ease of communication with maintainers to be defined (There is a google groups ML, which appears empty. A good first step would be to post there and introduce ourselves, establish the communication? ;)
• Audits and security reviews of pycryptodome: I haven't seen any trace, but it'd be a good question to ask the devs.
• And finally, asking pycryptodome maintainers what projects actually relying on their project they are aware of would be a good place to start?

Please keep in mind that all these criterias are not mandatory. I haven't set "weights" or "scores", so we can discuss the importance of these points together.

Regarding the counter mechanics, the way pycroptodome's counter increments does not fit OTRv3's specs, where we need two 8 bits halfs of counter that we increment and separately depending on exterior factors. One ugly way of doing would be to re-create the AES object in pycryptodome for each new message (with a new crafted counter). This is not optimal and might raise issues if, at some point, the last 8 bits overflow and the pycryptodome internal counter incrementation starts messing with the first half (from what I remember seeing on pycryptodome on tof of my head as I write this from my notes!)
Another solution would be to do as pycrypto did and implement our own counter mechanics, but we can't simply "drop-in" the counter implementation @afflux did on top of pycrypto.

When looking at this, @mathieui and I looked at how the libotr reference implementation handled its counters, and at first glance it handled counter reset and incrementation in a different way than either pycryptodome and the OTRv3 specs do. So either we misunderstood something in the convoluted C code libotr is (read: we're probably not very good at reading and understanding C!), or something in our implementations is lax enough so that communications do not break. From my notes the latter is most probable, as we exchange the first half of the counters over the wire (IIRC), having different ways of incrementing and resetting them would not break communication (even if one of the implementations interracting does not really respect the official specs)

So yeah, there is still a bit of work to go before we can "drop-in" pycryptodome, but for now I think it's stil lthe best course of action.
@mathieui already did some preliminary work in a branch, I'll have a look at his PR towards "staging"

I consider having the best unit test coverage we can an important part of this transition to detect breakages early, so I'll probably wait for unit tests to catch up before I actually merge pycryptodome integration anywhere near the master branch.

As always, everything said here is open to being discussed and I'd love to have additional feedback before we invest time and sweat on this option :)

[koolfy]

I had hoped for more opinions and debate, but it's fine :)

Just an update to confirm my intention to go forth with pycryptodome ASAP.
The way I see it:

1. Get to the bottom of libotr's counter managements
2. produce a longterm viable solution to pycryptodome's counter limitations (no temporary hack, we stick to what we do.)
3. have a working branch relying solely on pycryptodome
4. extend tests as much as we can, megrge them back in the pycryptodome branch to make sure they don't break
5. merge pycryptodome to the Staging branch
6. ping any project using POTR so they can try the staging branch and confirm that nothing dies
7. merge the pycryptodome to master (staging -> master) and release a new package to the world.

I'll try to have 1) done within this week if life allows it :)
2) should follow, but I'll move carefully. Counter bugs are pretty lethal with AES-CTR.

[claucece]

So, some thoughts around this:

There is a talk from the latest RWC, 2018 which talks about the different cryptographic libraries in python: https://www.youtube.com/watch?v=ALUq2Kc2YxA by Yasemin Acar.

It does include all the primitives we need for OTRv3 (but will lack ECCs defined in the latest informal OTRv4 draft I've seen on otr-dev mailing list)

But I see that Salsa and SHAKE are over there.

So, around how the counter works for OTRv3:

During the AKE, you set an initial counter value of 0 (of 16 bits).

After that, for each data message sent with the same sender keyid and recipient keyid pair, the top half of the counter should be incremented. It must not be all 0x00. When you receive a data message, its counter should be strictly larger than the last counter you saw using this pair of keys.

How it is incremented in the C code can be found here, and gets actually incremented here, just as you are planning to send a data message. It gets checked here, so you can check that this is not a replay.

I see that the checks are happening here at the python library. It seems to be increased with this function, which probably should be checked.

[koolfy]

Great digging!!

Somehow we must have missed otrl_dh_incctr at the time. So that clears things up.

Other great news:
I had a second look at pycryptodome CTR implementation and found that it does, in fact, allow for "external management" of the counter. At least in the sense that it allows us to specify an initial arbitrary value for the counter, AND a counter length, that will effectively separate the "counter prefix" from the "counter value to be incremented".

This allows us to setup the CTR mode with our strictly incremented session counter, as a prefix, specifying a proper length to separate this session counter from the rest (thus taking care of the fear the the block counter could overflow over to the session counter, pycryptodome would not allow it from what I understand.), and let pycryptodome handle the lower part of the counter itself, as it should.

I don't know if this changed "recently" or if we missed it too at the time, a quick look at the changelog indicates that the counter code has changed a few times recently, so I'll just pretend that pycryptodome heard us and added this feature at some point <3

TL;DR: I don't see any obstacle left to switching over to pycryptodome, the AESCTR suits our needs as far as I can tell, and having to handle new thrown exceptions and details will force us to clean our code anyway :) So that's probably a good thing.

This work will have to be done in a separate branch, that we SHOULDN'T merge until we are confident we have enough tests to be sure we didn't screw up something in a big way, and then probably leave this in a staging branch for a while for people to review and try to break it.

As a side-node, pycryptodome still lacks a ed448 implementation that's necessary to at some point attempt an OTRv4 (draft) implementation, but the lib is quite active so I feel confident that if we request this and promise to help with implementing it and testing it, pycryptodome would help, review and merge it.

This is still quite a lot of work, so let's focus on doing OTRv3 properly first.

I cleared up my weekend for 7-8 april to do some cleaning up, actually shipping a release with the latest fixes, and starting the work towards pycryptodome.

[koolfy]

Quick update to say that work towards using pycryptodome has started at last.
As expected it's not 100% drop-in, and a bunch of things crash right ouf of the box (not only the counter thing ;) ) but the tests written by @mmb @mathieui and @tribut are proving very useful in that task (I don't remember who wrote which tests where sorry!)

I'll be messing around these days, and you might even start seeing some commits in a separate branch (it'll break functionality almost completely for some time)

Very sorry about all the delays and the vaporware.

[koolfy]

In a dedicated branch (PLEASE DON'T USE IT FOR ANYTHING ELSE THAN TESTING, I BEG YOU), I started using pycryptodome's Counter "object" (now a dict, by the way?).
Most of our (still very simple) tests are passing with pycryptodome now.
c2c38da

The next issue now is a small difference in how pycryptodome exposes DSA internal values (x, y, p, q and g). https://github.com/Legrandin/pycryptodome/blob/master/lib/Crypto/PublicKey/DSA.py#L180
I expect a few more breakage around function calls that have been moved from DSA.stuff() to DSS.stuff(), and a buch more kabooms that our tests won't cover.

I'll add anything that breaks at runtime to the tests.

[claucece]

For some reason, I'm not getting github notifications, so I did not see this.

As a side-node, pycryptodome still lacks a ed448 implementation that's necessary to at some point attempt an OTRv4 (draft) implementation, but the lib is quite active so I feel confident that if we request this and promise to help with implementing it and testing it, pycryptodome would help, review and merge it.

Awesome! We finished the pure C implementation: https://github.com/otrv4/libgoldilocks

This is still quite a lot of work, so let's focus on doing OTRv3 properly first.

So, from OTRv3 I guess what is missing mostly are instance tags (from what I checked). Everything else seems to follow the spec.

This is still quite a lot of work, so let's focus on doing OTRv3 properly first.

This seems great!

[koolfy]

Long overdue news update.

I finally got back to it, and a few things stick out.
I don't know if something changed with pycryptodome or if I didn't properly test this out, but this test:

pure-python-otr/src/potr/compatcrypto/pycrypto.py

Line 53 in 1d51424

if set(counter) != set(Counter.new(64)):

doesn't work anymore.

It was a trick I got from discussions on IRC, relying on the fact that the counter object over at pycryptodome had become a python dict. I needed to validate its type, so I compared it with a set() function. Well this doesn't work anymore.

 src/potr/compatcrypto/pycrypto.py line 53 in AESCTR
      if set(counter) != set(Counter.new(64)):
   TypeError: '_counter.CounterBE' object is not iterable

Turns out it is a dict: https://github.com/Legrandin/pycryptodome/blob/master/lib/Crypto/Util/Counter.py#L65
But for some reason it can't be treated as such externally:

 >>> mycounter = Counter.new(64)
 >>> mycounter
 <_counter.CounterBE object at 0x7631e407ec90>
 >>> mycounter()
 '\x00\x00\x00\x00\x00\x00\x00\x01'
 >>> mycounter()
 '\x00\x00\x00\x00\x00\x00\x00\x02'
 >>> set(mycounter)
 Traceback (most recent call last):
   File "<stdin>", line 1, in <module>
 TypeError: '_counter.CounterBE' object is not iterable

I haven't found a way to properly validate this type yet, looking over at pycryptodome's, I see this: https://github.com/Legrandin/pycryptodome/blob/master/lib/Crypto/Cipher/_mode_ctr.py#L367
Which appears to be how pycryptodome validates the Counter type internally...
It's both weird and impossible for me to draw from, as I seemingly don't have access to counters internal variables (please correct me if I missed something on that front)

I don't feel comfortable avoiding this check as feeding weird types into the CTR mode sounds like a recipe for disaster, until I found a solution I commented out this check LOCALLY for my own test purposes, I will never intentionally commit the commented type check. If I ever do so, it's a mistake.
This allowed me to have a look at the next bunch of issues.

The first was trivial to fix:
1d51424
L72 and L73 both declare an all-zero counter the old way, this is fixed, I now generate those the right way with a proper prefix separation as well (8 all-zero bytes)

The next one not so much:

pure-python-otr/src/potr/crypt.py

Line 235 in 1d51424

sess.sendctr.inc()

Obviously counter.inc() is a call to the legacy potr-specific Counter methods, which don't exist anymore.
The bad news here is, as stated earlier, I don't seem to have access to internal values of the pycryptodome Counter object, despite it being a (normal?) python dict. I need to create a new counter with an incremented prefix value (the first 8 bytes), but to do so I must be able to get its current value.
I could maintain my own counter, external to the counter object, but a mistake there would be fatal. Both for compatibility and security. So I'll only do so if I find no other way... I might contact the pycryptodome team for pointers.

I didn't spot any test errors regarding the DSA issues anymore, maybe it has been fixed upstream to restore compatibility, but I didn't check further, I'll focus on that when the counters issues are sorted out.

As always:
DO NO USE THIS BRANCH FOR ANYTHING, EVEN IF YOU MAGICALLY GET IT TO WORK ON YOUR COMPUTER, IT'S EXPERIMENTAL AND PROBABLY DOES VERY STUPID MISTAKES

PS: my goal is to have the current potr working correctly through pycryptodome for christmas/35C3, so we can focus on finishing OTRv3 implementation and finally get moving towards OTRv4, I have no idea how far from that milestone I am right now :(

[koolfy]

Something very obvious hit me in the plane on my way back from 35C3 (no, not a bird.)

Remember when I said, on my previous post:

I don't know if something changed with pycryptodome or if I didn't properly test this out, but this test:
pure-python-otr/src/potr/compatcrypto/pycrypto.py
doesn't work anymore

Well, turns out my testing environment got contaminated by a mix of pycryptodome and the old pycrypto library, resulting in erratic behaviors, broken tests that actually weren't and compatibility issues that should not have been.

cleaned up the environment and the counter object type check works as expected, and I am actually back to the last issue I had previously identified: the new way of exposing DSA values in pycryptodome.

I am reverting commit 1d51424 for clarity and legibility, as tests now pass without this change as they should.

I am sorry for this confusion and time spend chasing ghosts, if anything it's a good reminder that there is no adult person in charge of this project, please don't use it for anything dangerous :(