fix(weaver): usage of weak PRNG issue #2907
Conversation
KaganCanSit
requested review from
petermetz,
takeutak,
izuru0,
jagpreetsinghsasan,
VRamakrishna,
sandeepnRES and
outSH
as code owners
|
Hello, If there is an easy way I can test it or if we can create the environment in a short time, I can make additional arrangements. Please check the current status when you are available. I'm just getting into the Open Source world. I can do my best as long as I have the opportunity. Some of the resources I used are; |
KaganCanSit
force-pushed
the
main
branch
6 times, most recently
from
7a877c2 to
bacf6f4
Compare
There was a problem hiding this comment.
@KaganCanSit Thank you very much for the detailed explanation and the context! Welcome on-board to our list of contributors!
If you ever have any more questions about contributing, we have daily pair programming call [1] that runs at 10 AM Pacific Time on most weekdays on our discord channel.
To answer your specific question as well and also give some tips, here's what I just did to figure out how to test the changes you made to certificate_utils.go
- I did a full text search in VSCode to
go buildwhich gave me a long list of hits - I found the first file that was a match to my search that also had the right path [2] as in, the go build runs in the directory where you changed the code.
- From then on, I saw that the
.github/workflows/test_weaver-go.yamlcontains what I need in terms of reproducing the test cases as they are executed on the CI - So what I did then is checked out your PR's branch in git and then ran these commands (basically just replaying what the github action in the .yml does)
cd weaver/core/network/fabric-interop-cc/contracts/interopgo build -v ./...go test -v ./...- The output of the test indicated that all the tests passed.
- Based on all the above, I think this is looking good.
[1] https://wiki.hyperledger.org/display/cactus/Daily+Pair+Programming+Calls
[2] 
There was a problem hiding this comment.
@KaganCanSit Sorry, a few more things in terms of administrative details:
- Please make sure to explain in the commit message thoroughly the what/why/how (weak encryption as the root cause is kinda self explanatory but the specific solution could be explained a little further so that everyone is on the same page about it for sure)
- Please use the
Fixes #???syntax in your commit message to indicate the issue number that you are fixing. It has to be exactly this way because then the bots pick it up and auto-close the issue that you are fixing the moment the pull request is merged (more automation makes it easier for the maintainers to review a higher number of pull requests and keeps everyone a little happier in general)
If you need any help with these things, especially the git related things, feel free to reach out for help here or in the pair programming calls that I was talking about in my previous comment as well.
KaganCanSit
changed the title
|
@petermetz Thanks for the information you shared with me. I tried to explain the logic of the error and why I made the change as briefly as I could. I included the error code in my message in the required format. If there is any content you would like me to edit, please write. I will deal with it at my first available time. |
There was a problem hiding this comment.
@KaganCanSit You were editing it only in one place but it has to be done in two places (unfortunately the way GitHub works for some reason).
You need to amend the git commit message (right side of screenshot) with the same content you put in the pull request description (left side of the screenshot).
The reason behind this is that the pull request description is something that is stored in a proprietary database that we don't have full access to (GitHub's own servers) whereas the git log is something that is native to git itself and should we ever end up migrating away from GitHub to a different platform we could do so knowing that the important historical context of the changes we've made can be found in the git log, not just the database powering the website called github.com
KaganCanSit
force-pushed
the
main
branch
3 times, most recently
from
a99c681 to
1102e7b
Compare
|
@petermetz Sory,I should have remembered to amend the commit message. It seems okay now. |
The Logic Behind the Problem When RNG (Random Number Generator) values are not received through a hardware TRNG, seed values apply a certain pattern. (It takes a seed value such as a mathematical formula or time.) In response to this situation, there are various secure random classes to increase security. Solution Changes have been made to get random values using safe randomness instead of mathematical randomness. This increases the complexity of the pattern, making it difficult to discover even if data is listened to for long periods of time. The changes that have been made; - In the certificate_utils.go file, the random value was taken from the math class (mrand math/rand) and used. By taking this random value from the secure random class, we obtain a more reliable random value. I added HmacGenerate and generateSecureRandomKey functions for readability and ease of use. If you want to generate a key again, the generateSecureRandomKey function, which uses secure random, can be used. - In HashFunctions.kt, kotlin.random.Random class has been replaced with the more reliable java.security.SecureRandom class. - The reason for the change in eciesCrypto.js is that the length of aes-128-ctr is not considered reliable by various standards. For this reason, I preferred the more reliable 256 length. Fixes hyperledger-cacti#2765 Signed-off-by: Kağan Can Şit <[email protected]>
|
@KaganCanSit One of the changes made in this PR was incorrect, causing the code to fail. The unit test for that module would have caught the error but was disabled. Can you please work on a fix ASAP? I've added appropriate comments against the offending change. @petermetz @sandeepnRES FYI. |
Can you point to a source for this? I need to understand the implications. Per my understanding, 128 bits for AES is still highly secure. (But my knowledge could be outdated.) |
|
I checked with a cryptography expert and he informed me that 128-bit AES is secure enough for classical computers but is quantum-unsafe. To be quantum-safe, we do need to upgrade to 256 bits. |
|
Hello @VRamakrishna, First of all, I would like to help you and learn something new as soon as possible. Since this is my first contribution to this project, I may have missed some parts. I would like to start by explaining as best I remember why I changed the AES-128 length to AES-256. When I examined the content of the opened issue (https://github.com/hyperledger/cacti/issues/2765), I saw that mostly the random seed values were not from reliable sources. Basically, the first two parts mentioned were like this. However, the "crypto" class was used in the "weaver/sdks/fabric/interoperation-node-sdk/src/eciesCrypto.js" file and was specified as weak. When I examined various sources, I thought that the AES-128-CTR key was mentioned as the part that needed to be replaced. This was the first idea that led me to increasing the key length. I took a look at various contents. Basically, in many sources, the length of 128 is considered sufficient, as you said. Including in banking. (https://www.pcisecuritystandards.org/glossary/strong-cryptography/) In other cases, there is information that it was increased as a precaution against Quantom computers and military content. They think it would be better to use 256 to preserve data over time.(https://crypto.stackexchange.com/questions/76738/has-aes-128-been-complete-broken/76746#76746, Since I'm a C++ developer, I was a little hesitant about this pr. For testing I relied on the build states and tests on the path @petermetz outlined for me.
It's been a busy week, but I can use my weekend to try to improve. I would be very happy if you leave various instructions and little tips. Please share your knowledge with me. Have a nice day. |
@VRamakrishna Sorry, this is totally my fault! I still haven't re-enabled requiring the tests because the optimizations on the CI that would bring down the resource usage to acceptable levels are still pending. I'll work on tightening back up the CI because this is definitely a wake-up call in that sense. |
|
@KaganCanSit Thanks for your explanation. Really appreciate your enterprise in finding potential vulnerabilities in the code base and trying to fix them. My comments were not meant as criticism of your effort and initiative but rather the completion of the task. The main problem was that the relevant test was disabled. If that wasn't the case, you'd have been alerted to the fact that more changes were needed in the code after your 128-->256 bit modification. The right steps to test the module you touched are as follows:
The above steps are coded in the GitHub Action I think the change to AES-256-CTR does need to be done (based on long-term security concerns), but you can take your time with it. For now, to avoid inconsistencies, I'm going to submit a PR reverting just that change (the rest of your changes in this PR looked good to me.) |
@petermetz Don't worry about it. I understood the rationale for disabling many of the tests earlier, but perhaps it's time to re-enable the mandatory unit tests now so problems like these don't slip through the cracks. |
|
@KaganCanSit @petermetz I opened a PR which should fix this issue by making the key length configurable: https://github.com/hyperledger/cacti/pull/2953. |
|
@VRamakrishna thank you for sharing your knowledge with me. I know you don't criticize me and you guide me. As an engineer candidate at the beginning of my journey, I think I will learn a lot from the open source world that will broaden my horizons. I respect your knowledge and effort. I will try to research and make a correction for my mistake here as soon as possible. |
The Logic Behind the Problem
When RNG (Random Number Generator) values are not received through a hardware TRNG, seed values apply a certain pattern. (It takes a seed value such as a mathematical formula or time.) In response to this situation, there are various secure random classes to increase security.
Solution
Changes have been made to get random values using safe randomness instead of mathematical randomness. This increases the complexity of the pattern, making it difficult to discover even if data is listened to for long periods of time.
The changes that have been made;
In the "certificate_utils.go" file, the random value was taken from the math class (mrand "math/rand") and used. By taking this random value from the secure random class, we obtain a more reliable random value. I added HmacGenerate and generateSecureRandomKey functions for readability and ease of use. If you want to generate a key again, the generateSecureRandomKey function, which uses secure random, can be used.
In "HashFunctions.kt", kotlin.random.Random class has been replaced with the more reliable java.security.SecureRandom class.
The reason for the change in "eciesCrypto.js" is that the length of aes-128-ctr is not considered reliable by various standards. For this reason, I preferred the more reliable 256 length.
Fixes #2765
Pull Request Requirements
[☑] Rebased onto
upstream/mainbranch and squashed into single commit to help maintainers review it more efficient and to avoid spaghetti git commit graphs that obfuscate which commit did exactly what change, when and, why.[☑] Have git sign off at the end of commit message to avoid being marked red. You can add
-sflag when usinggit commitcommand. You may refer to this link for more information.[ ] Follow the Commit Linting specification. You may refer to this link for more information.
Character Limit
A Must Read for Beginners
For rebasing and squashing, here's a must read guide for beginners.