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 :(

[dngray]

Is this still happening? I found this issue came up with a conflict between two weechat plugins. Namely poljar/weechat-matrix and this one. See poljar/weechat-matrix has poljar/matrix-nio which depends on Legrandin/pycryptodome which of course conflicts with python-otr/pure-python-otr dependence on dlitz/pycrypto.

[koolfy]

Short answer: YES! Long answer: I'm short on spare time right now and over the past few months, made worse by the tragic loss of my raspberry pi serving as my IRC/XMPP client. I need to set things up once again, reappear in the OTR channels and resume work on the pycryptodome migration that was quite far ahead IIRC. I've been dragging this for incredibly long, help would really be welcome... It gets lonely and demotivating :( I hope I can post some progress over this weekend. I think most people need a proof of life or they might call the cops ^^' SORRY. Excerpts from Daniel Nathan Gray's message of September 29, 2019 7:28 pm:

…

Is this still happening? I found this issue came up with a conflict between two weechat plugins. Namely [poljar/weechat-matrix](https://github.com/poljar/weechat-matrix) and this one. See [poljar/weechat-matrix](https://github.com/poljar/weechat-matrix) has [poljar/matrix-nio](https://github.com/poljar/matrix-nio) which depends on [Legrandin/pycryptodome](https://github.com/Legrandin/pycryptodome) which of course conflicts with [python-otr/pure-python-otr](https://github.com/python-otr/pure-python-otr) dependence on [dlitz/pycrypto](https://github.com/dlitz/pycrypto). -- You are receiving this because you commented. Reply to this email directly or view it on GitHub: #68 (comment)

[dngray]

Short answer: YES!

This is great news. Having to choose between weechat-otr or weechat-matrix sucks big time.

Long answer: I'm short on spare time right now and over the past few
months, made worse by the tragic loss of my raspberry pi serving as my
IRC/XMPP client. I need to set things up once again, reappear in the OTR
channels and resume work on the pycryptodome migration that was quite
far ahead IIRC.

It's nice to know you're still alive 👍 I looked at your github commit history and thought something bad might have happened.

I've been dragging this for incredibly long, help would really be
welcome... It gets lonely and demotivating :(

I do get that, especially as anyone who's developing these days is doing so for Matrix. Things like OTR are not all that 'cool'. Matrix does still have a few rough edges and OTR can be nice for simplicity sake.

I also have a few friends on IRC I use this in 1:1 with. One of them is completely blind (and has been since birth), he uses irssi's otr support. Adopting new software is a very complex task for him so it would be great to still have this to keep in touch.

I would help if I can. I have to admit I don't know a lot about crypto specifics and my python abilities are rudimentary. If there's something you want me to do, I'll try my best to look into it.

I hope I can post some progress over this weekend.
I think most people need a proof of life or they might call the cops ^^'

Well it's nice to know you're in good health still. I can think of a couple of examples in the past where developers have 'fallen off the face of the earth' to later find out they had serious mental breakdown issues or died somehow.

[aidecoe]

Maybe the patch would be of a help?

From ce6a4be79beeda9d378204f37024965a85373865 Mon Sep 17 00:00:00 2001
From: Daniel Robbins <[email protected]>
Date: Tue, 19 Nov 2019 22:33:18 +0000
Subject: [PATCH] Support pycryptodome

---
 README.md                         | 4 ++--
 requirements.txt                  | 2 +-
 setup.py                          | 6 +++---
 src/potr/compatcrypto/pycrypto.py | 4 ++--
 4 files changed, 8 insertions(+), 8 deletions(-)

diff --git a/README.md b/README.md
index 1f82628..ae3003b 100644
--- a/README.md
+++ b/README.md
@@ -8,14 +8,14 @@ Install the potr Python module:
 
     sudo python setup.py install
 
-__Dependencies__: pycrypto >= 2.1 (see [dlitz/pycrypto](https://github.com/dlitz/pycrypto))
+__Dependencies__: pycryptodome
 
 This software is experimental and potentially insecure. Do not rely on it
 =========================================================================
 
 Usage Notes
 ===========
-This module uses pycrypto's RNG. If you use this package in your application and your application
+This module uses pycryptodome's RNG. If you use this package in your application and your application
 uses `os.fork()`, make sure to call `Crypto.Random.atfork()` in both the parent and the child process.
 
 Reporting bugs
diff --git a/requirements.txt b/requirements.txt
index 03917ad..acdfd20 100644
--- a/requirements.txt
+++ b/requirements.txt
@@ -1 +1 @@
-PyCrypto >= 2.1
+pycryptodome
diff --git a/setup.py b/setup.py
index 5618f9c..a5c8099 100644
--- a/setup.py
+++ b/setup.py
@@ -22,7 +22,7 @@ try:
 
     from setuptools.command.install_lib import install_lib
 
-    args['install_requires']=['pycrypto>=2.1']
+    args['install_requires']=['pycryptodome']
 except ImportError:
     print('\n*** setuptools not found! Falling back to distutils\n\n')
     from distutils.core import setup
@@ -45,11 +45,11 @@ Install the potr Python module:
 
     sudo python setup.py install
 
-**Dependencies**: pycrypto >= 2.1 (see `dlitz/pycrypto <https://github.com/dlitz/pycrypto>`_)
+**Dependencies**: pycryptodome
 
 Usage Notes
 ===========
-This module uses pycrypto's RNG. If you use this package in your application and your application
+This module uses pycryptodomes's RNG. If you use this package in your application and your application
 uses ``os.fork()``, make sure to call ``Crypto.Random.atfork()`` in both the parent and the child process.
 
 Reporting bugs
diff --git a/src/potr/compatcrypto/pycrypto.py b/src/potr/compatcrypto/pycrypto.py
index 58a8577..9c37d6a 100644
--- a/src/potr/compatcrypto/pycrypto.py
+++ b/src/potr/compatcrypto/pycrypto.py
@@ -20,7 +20,7 @@ try:
 except ImportError:
   import crypto as Crypto
 
-from Crypto import Cipher
+from Crypto.Cipher import AES
 from Crypto.Hash import SHA256 as _SHA256
 from Crypto.Hash import SHA as _SHA1
 from Crypto.Hash import HMAC as _HMAC
@@ -48,7 +48,7 @@ def AESCTR(key, counter=0):
         counter = Counter(counter)
     if not isinstance(counter, Counter):
         raise TypeError
-    return Cipher.AES.new(key, Cipher.AES.MODE_CTR, counter=counter)
+    return AES.new(key, AES.MODE_CTR, counter=counter)
 
 class Counter(object):
     def __init__(self, prefix):
-- 
2.21.0

[koolfy]

Short answer: YES!

This is great news. Having to choose between weechat-otr or weechat-matrix sucks big time.

I can imagine, this is not how things should go :(

Long answer: I'm short on spare time right now and over the past few
months, made worse by the tragic loss of my raspberry pi serving as my
IRC/XMPP client. I need to set things up once again, reappear in the OTR
channels and resume work on the pycryptodome migration that was quite
far ahead IIRC.

It's nice to know you're still alive +1 I looked at your github commit history and thought something bad might have happened.

Nothing of the sort! Don't worry :)

I've been dragging this for incredibly long, help would really be
welcome... It gets lonely and demotivating :(

I do get that, especially as anyone who's developing these days is doing so for Matrix. Things like OTR are not all that 'cool'. Matrix does still have a few rough edges and OTR can be nice for simplicity sake.

Check out OTRv4, it's cool as all hell ;) Can't wait to finally be able to work on implementing it here!

I also have a few friends on IRC I use this in 1:1 with. One of them is completely blind (and has been since birth), he uses irssi's otr support. Adopting new software is a very complex task for him so it would be great to still have this to keep in touch.

I can understand that, at least I know libotr (used for irssi integration) is in a much better state than this python implementation, so you friend is in good hands ;)

I would help if I can. I have to admit I don't know a lot about crypto specifics and my python abilities are rudimentary. If there's something you want me to do, I'll try my best to look into it.

Sadly the current roadblocks are a bit of python wizardry. Not even advanced crypto stuff for now (but everytime this work forces me to look closer at code I didn't write in here I make sure to check that I don't find security issues).
It's slow boring but necessary work, and the worst part is until I get it working at all I can't even ask you to proof test it :( But that might come in very handy at later stages so please stick around if you can!

I hope I can post some progress over this weekend.

(this person lied. I should know, I am that person .___.)

I think most people need a proof of life or they might call the cops ^^'

Well it's nice to know you're in good health still. I can think of a couple of examples in the past where developers have 'fallen off the face of the earth' to later find out they had serious mental breakdown issues or died somehow.

To be honest I am in constant partial-burnout on this project for a few years now. Everytime I touch it I relapse and hide from it in shame for a few months. Allocating time for it is hard enough, but running away from it makes everything worse.

People at the #OTR irc channel over at irc.oftc.net (and the other parts of this community) are always in touch and within shouting distance to make sure I'm okay. This really is a great community, which makes disappointing them all the more painful :(

I'M OKAY, I'm not in any serious danger. When I smell burnout I protect myself and this project just gets a few more weeks of inactivity ^^'
But this cycle needs to stop. I need to get some momentum going and start a positive feedback loop.

That's going to be it for this post, I don't really enjoy whining but I thought some transparency was in order as to the reasons for the lack of progress and activity.
I also, as you do, have heard to many painful stories to not take mental health seriously, so I'm being as honest as I can.

I'm not in any danger, but this project is, and I really, really still want to save it.

Ok, let's get back to the technical part and get some actual shit done.

[koolfy]

@aidecoe Hey! Thanks for dropping by!

Actually, when I test my current pycryptodome work branch ( https://github.com/python-otr/pure-python-otr/tree/bugfix/issue%2368-use-pycryptodome ) I handle the pycryptodome dependance manually to make 100% sure I don't mess with my test environment again, but your changes will come in handy once the branch works, thanks!

As for the AES part, actually this modification isn't necessary, looking at my pycryptodome branch you can see the import and function calls don't need to change https://github.com/python-otr/pure-python-otr/blob/bugfix/issue%2368-use-pycryptodome/src/potr/compatcrypto/pycrypto.py#L47
The AES part looks functionnal for now, at least the tests pass so we'll see once it runs in real life, but I don't expect it to cause any more troubles for now :)

Thanks for the help though!

[koolfy]

I really should be in bed by now but I couldn't just leave it at posting comments without showing some progress ;)

So, first thing, I'm working on the DSA signature now.
Back in january 1st, I quickly fixed the different access to internal x,y,g,p,q values:
09f55c4

I also modified the DSA signing function to conform to the new way signatures are called:
9b382ef
(it's also cool that the pycryptodome guys figured out it's unsafe to rely on the developers to pick and generate a safe K value for no justifiable reason. One less thing to worry on my side. I really like this library.)

Don't get too excited about that last commit though:

• It's not new code, I literally found this uncommited code dating back to jan 1st in my workdir:

-rw-rw-r-- 1 user user 4.2K Jan 1 2019 pycrypto.py

• It doesn't work (yet)

The reason it doesn't work is that, along with other cool ideas, pycryptodome developpers actually check that all input for DSA signatures are hash values. they do that by checking that the input for any DSA "message" to sign has a specific "oid" attribute containing a very specific hash identifier.
This way pycryptodome can tell not only if the string about to be signed is actually a hash, it can also check that the hash is in a list of sane hashes and check that were not shooting ourselves in the foot.

This is good.
But it means I can't use any other hash function other than the ones provided by pycryptodome (who will create a proper object with an "oid" field the DSA signature function will recognize).

Currently, the input we feed the DSA signing function is a plain string, thus I get the following exception:

src/potr/crypt.py line 472 in calculatePubkeyAuth
  MB = self.privkey.sign(SHA256HMAC(mackey, buf))
src/potr/compatcrypto/pycrypto.py line 89 in sign
  signature = signer.sign(data)
/home/user/.local/lib/python2.7/site-packages/Crypto/Signature/DSS.py line 96 in sign
  if not self._valid_hash(msg_hash):
/home/user/.local/lib/python2.7/site-packages/Crypto/Signature/DSS.py line 277 in _valid_hash
  return (msg_hash.oid == "1.3.14.3.2.26" or
AttributeError: 'str' object has no attribute 'oid'

This would in theory not be that much work, but the issue is actually a bit more tricky than that.
We're already using native pycryptodome hash functions.
But we're not hashing that.
The code fails because:

src/potr/crypt.py line 472 in calculatePubkeyAuth
  MB = self.privkey.sign(SHA256HMAC(mackey, buf))

We're signing the result of a SHA256HMAC(), which does not bear an "oid" attribute, thus the DSA signature fails as it believes it's not signing a proper hash.

Basically, pycryptodome's logic allows for DSA signatures on pure hash outputs, but not HMAC outputs.

This is the wall I hit on january 1st and the reason why I initially didn't commit my workdir changes.

I might be mistaken, bear in mind I'm slowly recollecting my thought process from almost one year ago, and it's 2AM in the morning in between two exhausting workdays, so I my description might be a little off, I'll make edits later if necessary :)

I don't think this is going to ba a huge roadblock, I'll re-check that my analysis is correct, that there is not a simple fix I could do that would make a lot more sense. If not, I'll contact the pycryptodome people, this looks like a very simple omission on their part and I doubt it would be hard for them to fix it in a backwards-compatible way :)

I'll stop there for tonight, but this is not the last you'll hear of this!
Thanks again for the support and the kind words ;) <3

[dngray]

To be honest I am in constant partial-burnout on this project for a few years now. Everytime I touch it I relapse and hide from it in shame for a few months. Allocating time for it is hard enough, but running away from it makes everything worse.

People at the #OTR irc channel over at irc.oftc.net (and the other parts of this community) are always in touch and within shouting distance to make sure I'm okay. This really is a great community, which makes disappointing them all the more painful :(

I'M OKAY, I'm not in any serious danger. When I smell burnout I protect myself and this project just gets a few more weeks of inactivity ^^'
But this cycle needs to stop. I need to get some momentum going and start a positive feedback loop.

That's going to be it for this post, I don't really enjoy whining but I thought some transparency was in order as to the reasons for the lack of progress and activity.
I also, as you do, have heard to many painful stories to not take mental health seriously, so I'm being as honest as I can.

I'm not in any danger, but this project is, and I really, really still want to save it.

Ok, let's get back to the technical part and get some actual shit done.

hey, appreciate the update. Stay in good health!

Basically, pycryptodome's logic allows for DSA signatures on pure hash outputs, but not HMAC outputs.

This is the wall I hit on january 1st and the reason why I initially didn't commit my workdir changes.

I also appreciate the commentary as it allows us to know whats going on (and learn something from your experience. We might even perhaps get people who might know the answers to apply their experience.

I might be mistaken, bear in mind I'm slowly recollecting my thought process from almost one year ago, and it's 2AM in the morning in between two exhausting workdays, so I my description might be a little off, I'll make edits later if necessary :)

This is what happens when you don't write things down 😀.

I don't think this is going to ba a huge roadblock, I'll re-check that my analysis is correct, that there is not a simple fix I could do that would make a lot more sense. If not, I'll contact the pycryptodome people, this looks like a very simple omission on their part and I doubt it would be hard for them to fix it in a backwards-compatible way :)

I'll stop there for tonight, but this is not the last you'll hear of this!
Thanks again for the support and the kind words ;) <3

Awesome, keep up the good work. This ticket should also help to remind you if you forget later on or get stuck on something.

[edg-l]

Please use https://github.com/pyca/cryptography instead, pycrypto is broken on python 3.8

[dngray]

@koolfy made any progress how's things?

Please use https://github.com/pyca/cryptography instead, pycrypto is broken on python 3.8

@Ryozuki Looks like that was fixed.

3.9.2 (10 November 2019)

New features

• Add Python 3.8 wheels for Mac.

3.9.1 (1 November 2019)

New features

• Add Python 3.8 wheels for Linux and Windows.

There's been a bunch of releases since then, including one 4 days ago.

[koolfy]

@dngray I am resuming work right now, sorry for the delay, things got complicated and crowded on my side of things, I'll be posting an update and maybe even good news very shortly.

Current status is: "koolfy has 1kg of yerba mate, a long weekend and a cleared up schedule (at last!)"

[koolfy]

All right, there was no sane way of working around this, I took a long time to look at pycryptodome and how we were using it to be certain of my understanding of the situation before creating an issue upstream, but there it is.

Right now we depend on the outcome of this discussion over at pycrypotodome to move forward, I'll keep you posted as soon as a branch or a commit comes up on their side allowing me to make something work.

In any case some minor changes will be needed on my side as well to accomodate for this change upstream, as we pretty much immediately collapse the HMAC object into its binary string attribute: https://github.com/python-otr/pure-python-otr/blob/master/src/potr/compatcrypto/pycrypto.py#L44
This is as simple as carefully removing the .digest() call there and add it at the right places elsewhere in the code where the .digest() output is expected. This is to allow the raw HMAC object to get to the signing code where it need to still have a .oid attribute, once pycryptodome exposes it.

I have local code doing just that right now, not worth a commit, but waiting for the .oid attribute :)

I'll state it again, without @mmb 's contributions and tests, every single step in this process would be stepping on a minefield, the help it provides me since day 1 cannot be overstated. I would probably not even dare with this migration, as these simple tests caught many stupid mistakes on my part.

If you want to help in the meantime, I'd say the single most valuable thing I could merge right now is more tests that you might think could be missing and useful. Maybe I should measure code coverage to have a better idea of what more could be added.

[dngray]

This is awesome news, and I'm glad you've persevered! Many will be happy to have this functionality regained.

[koolfy]

Very short progress report:

From the pycryptodome github issue, I was told that the RFC for the signature method I was using did not actually allow for HMACs to be used as input data, while deterministic signatures (RFC-6979) should.

This raises two questions:

1. would using this deterministic signature work as code
2. would this in any way break the OTR protocol's implementation

on 2), I suspect not, but I'll need to be certain, haven't checked yet, as 1) is the bad news here

What should have been a very quick (if late) test to finally plow through this signature issue turned into an antire bug-hunt evening within pycryptodome.

I won't duplicate the boring explanation here, but you can read the details there: Legrandin/pycryptodome#414 (comment)

TL;DR: as it turns out, pycryptodome doesn't actually allow signing HMAC objects in this way, because of a data structure handling cleverness, the code actually expects the input data to be a naked hash object, and a hmac construct will break that assumption. This shouldn't be a blocking issue, but I'm unsure of the right way to work around this, so I won't submit an ugly PR and let the maintainer come up with an elegant solution.

If I'm not dead wrong on this bug it should be resolved quickly, my main worry is that my comment might not get noticed, as I can't reopen the bug :p

It would be cool to be the first ones to run into this bug, bonus points for contributing to upstream libraries \o/

I'll keep you posted in less than a month this time, I swear.

[eamanu]

Hi,

Just to be notice, on Debian, PyCrypto going to be removed (that is the plan) [0]

And exist a patch to Depends on pycryptodome [1] [2]

Not uploaded yet

cheers

[0] https://bugs.debian.org/cgi-bin/bugreport.cgi?bug=971306
[1] https://salsa.debian.org/python-team/packages/python-potr/-/commit/47c6afefe99b03db9b4aa505bde4b3ab65183f27
[2] https://salsa.debian.org/python-team/packages/python-potr/-/commit/10895982d99a42c44d49137dd518cfe5795cbc60

[eamanu]

Hi everybody,

there's some advances on this issue?

Cheers,

[koolfy]

@eamanu
Hello, sorry I saw your message the day you posted it and wanted to find a time slot that would allow me to check the situation, possibly run some tests/do some work, before giving you a proper answer.

I have found that time and courage, unfortunately the conclusion is quite quick:
I'm stuck trying to get attention to: Legrandin/pycryptodome#493
which is blocking me from implementing OTR2/3 correctly on top of pycryptodome

I don't feel competent to write a pull request to pycryptodome that would have a chance of getting accepted and I am perfectly capable of introducing devastating vulnerabilities in pycryptodome if left unchecked (maybe stupidities that would survive their review? Certainly motivation enough to not package a cystom pycryptodome fork by myself in any way to "just make potre work on some environments", so that's a non-starter forme).

Again, I am mostly at fault for the state of this lib right now. I dont want to give up though, I really want to see this through, and reach OTRv4 implementation some day.
I need help though, and right now help means either an improvement of the crypto primitives lib upstream, or someone pointing out to me what I missed and how stupid I am for not having noticed this other way of doing things that entirely solves the entire issue :D

I would love nothing more than looking like a fool, if it means releasing a feature-complete OTRv2/3 python implementation built on top of a crypto primitives lib that is actually still maintained/packaged/shipped.

[koolfy]

Progress has been made by bit and a PR was merged that should finish the pycryptodome integration.

I need to test it some more in its current form before this can be merged back progressively towards a new main stable version that can be released :)
I'm hopeful again!

Maybe when this is sorted out I can get moving again towards full OTRv3 compatibility and look at OTRv4 feasibility!

[eggbean]

The latest version of Alpine Linux (3.18) no longer can build wheel for pycrypto. Is pycryptodome support likely?