Multiple issues with Branca spec conformance

[brycx]

I've been looking through the Branca implementation and seem to have found several issues. Branca is not implemented as defined in the specification. This issue would have surfaced much earlier, had there been test vectors from the other implementations.

XChaChaEngine and Poly1305 does not implement XChaCha20-Poly1305. XChaCha20-Poly1305 is implemented with HChaCha20 and ChaCha20-Poly1305. Please see the draft RFC and RFC 8439.

The Poly1305 authentication tags are not compared in constant-time, and can therefore leak information to an attacker through a timing side-channel.

[scottbrady91]

You know what, I've just seen your article and that really fucking hurt.

I would have loved to have collaborated. I would have loved to have received a PR containing some of the code you have addressed. I would have loved some responsible disclosure.

You opened these issues at the same time you released an article. Please consider reaching out first next time and helping someone improve their personal project.

I'll see about addressing some of your comments asap but I'd really appreciate it if you took down that article.

[scottbrady91]

I have updated this library's implementation of Branca to use libsodium as opposed to Bouncy Castle. The issues you have described most likely affect all other Branca implementations using Bouncy Castle. For instance, this library ported the implementation found in the Java libraries and used test data generated by them.

Again, I really want to stress the importance of responsible disclosure and working with developers to fix issues you might find, as opposed to immediately discrediting them. This might be a personal project of mine where I ported some Java code, but I'd still appreciate some warning before being targetted by an article like yours. I again request that you remove your article.

[brycx]

I would have loved some responsible disclosure.

There were mainly two reasons I opted for full disclosure. The first was that the NuGet seemed relatively new and unknown, few downloads and without any users. I searched around for users of this Branca implementation but found none (of course that doesn't mean there are none at all). I would've taken a different approach if it were more popular and used. Second, there was no information on how to report security vulnerabilities. I checked both this repository and the site of the blogpost linked, and I found nothing.

I see you've just added a SECURITY.md. Thanks for this. This will make it much clearer to people on how to handle situations like these in the future. I simply assumed, from the lack of policy, that you didn't care.

In hindsight, I should probably have tried to contact you to ask what you'd have preferred. I apologize for not having done so and I will keep that in mind for the future.

Please consider reaching out first next time and helping someone improve their personal project.

My post and these issues are an attempt to help. Both in making it clear what needed to be fixed but also how others can avoid similar situations in the future (main motivation behind the post). The latter will not be possible if the post is taken down.

I have updated this library's implementation of Branca to use libsodium as opposed to Bouncy Castle.

I just ran it through the first few tests from the JS implementation, and can confirm they now pass.

The issues you have described most likely affect all other Branca implementations using Bouncy Castle. For instance, this library ported the implementation found in the Java libraries and used test data generated by them.

If that is the case, the issues also need to be reported there of course.

[scottbrady91]

I am struggling to see your side of the story here. I get what it's like to get wound up by someone else's code, especially if their mistakes are so obvious to you, but you should be trying to help, as opposed to gatekeeping and discrediting. I'm sorry that I upset you enough to take that route, but again, it hurts to be attacked out of nowhere.

I think it's obvious that I care based on how much this hurt and how fast I addressed the issues.

If you genuinely think there is value in your article, I suggest making it more generic (e.g. Bouncy Castle), removing my name from it, and switching to a more analytical tone. I think you owe me that.

[brycx]

I think it's obvious that I care based on how much this hurt and how fast I addressed the issues.

Let me clarify: I assumed you didn't care about how security issues were reported. I wasn't sure if we were talking about the same thing.

If you genuinely think there is value in your article, I suggest making it more generic (e.g. Bouncy Castle), removing my name from it, and switching to a more analytical tone. I think you owe me that.

I fail to see how Bouncy Castle has anything to do with this. Further, I'm left with little choice when the NuGet is released with a name that includes your own. There are many more popular NuGets listed, with millions of downloads, when searching for "IdentityModel" on nuget.org. To not specifically name yours fully throughout the entire post, but still not create confusion about what NuGet I was talking about, I also chose to shorten it:

"I’ll also be referring to this NuGet as just IdentityModel throughout the rest of this post."

My post outlines how severe issues can occur when basic testing is ignored, through critiquing a publicly released NuGet. Had there been similar NuGets, it could have been any of them. I don't see how I've done you wrong in this regard or owe you anything.