-
Notifications
You must be signed in to change notification settings - Fork 22
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Fix #485: Crypto 3.2: Prevent replay attacks #486
Conversation
…, parameterize presence of timestamp instead
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good, just minor comments to consider.
...ain/java/io/getlime/security/powerauth/crypto/lib/encryptor/ecies/model/EciesCryptogram.java
Outdated
Show resolved
Hide resolved
...rauth-java-crypto/src/main/java/io/getlime/security/powerauth/crypto/lib/util/ByteUtils.java
Outdated
Show resolved
Hide resolved
...auth-java-crypto/src/main/java/io/getlime/security/powerauth/crypto/lib/util/EciesUtils.java
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I found some minor issues and one flaw in my specification that has to be addressed.
...rauth-java-crypto/src/main/java/io/getlime/security/powerauth/crypto/lib/util/ByteUtils.java
Outdated
Show resolved
Hide resolved
...ain/java/io/getlime/security/powerauth/crypto/lib/encryptor/ecies/model/EciesCryptogram.java
Outdated
Show resolved
Hide resolved
...auth-java-crypto/src/main/java/io/getlime/security/powerauth/crypto/lib/util/EciesUtils.java
Outdated
Show resolved
Hide resolved
...auth-java-crypto/src/main/java/io/getlime/security/powerauth/crypto/lib/util/EciesUtils.java
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
While the intentions of the change are well reasoned and prevent the issue on a system level, I would not write the code 1:1 according to the specification because the resulting protocol seems to diverge from formal ECIES description too much. Instead, I would adjust the code so that:
- core ECIES is unchanged
- additional values (additional data, timestamp, ...) are mapped to SharedInfo1 and SharedInfo2
- EciesCryptogram object is split to "body" (encrypted payload, ephemeral public key, mac) and "headers" (timestamp, nonce, context values such as additional data, etc.)
...rauth-java-crypto/src/main/java/io/getlime/security/powerauth/crypto/lib/util/ByteUtils.java
Show resolved
Hide resolved
...rauth-java-crypto/src/main/java/io/getlime/security/powerauth/crypto/lib/util/ByteUtils.java
Outdated
Show resolved
Hide resolved
...rauth-java-crypto/src/main/java/io/getlime/security/powerauth/crypto/lib/util/ByteUtils.java
Outdated
Show resolved
Hide resolved
...rauth-java-crypto/src/main/java/io/getlime/security/powerauth/crypto/lib/util/ByteUtils.java
Outdated
Show resolved
Hide resolved
...auth-java-crypto/src/main/java/io/getlime/security/powerauth/crypto/lib/util/EciesUtils.java
Outdated
Show resolved
Hide resolved
...o/src/main/java/io/getlime/security/powerauth/crypto/lib/encryptor/ecies/EciesEncryptor.java
Outdated
Show resolved
Hide resolved
...o/src/main/java/io/getlime/security/powerauth/crypto/lib/encryptor/ecies/EciesEncryptor.java
Show resolved
Hide resolved
…e usage, update tests, migrate to protocol V3.2 parameters
… vectors from mobile SDK are available
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks OK
The updated ECIES implementation is ready for review. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good.
...c/main/java/io/getlime/security/powerauth/crypto/lib/encryptor/ecies/model/EciesPayload.java
Show resolved
Hide resolved
...c/main/java/io/getlime/security/powerauth/crypto/lib/encryptor/ecies/model/EciesPayload.java
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Look OK (change about moving deriveAssociatedData()
to this library)
Alright, I will merge this pull request and I will publish the updated powerauth-crypto library after merge |
No description provided.