MetaMask / snaps-registry

A registry containing metadata about verified and blocked Snaps.
Apache License 2.0
31 stars 16 forks source link

[New Snap] Web3MQ #132

Closed Montoya closed 1 year ago

Montoya commented 1 year ago

Checklist

All items in the list below needs to be satisfied.

Montoya commented 1 year ago

List of findings:

Issue A: Weak Key Derivation Algorithm Used

Fixed in https://github.com/Generative-Labs/Web3MQ-Snap/commit/f03fa7e48c26ede4ac3ff6dc09463acfed748a07

Issue B: Unnecessary Usage of Non-Standard Libraries

This is a minor issue and does not need to be resolved.

Issue C: Initialization Vector Used Does Not Meet Recommended Best Practices

The Generative Labs team stated that decoding and encoding require consistent IV parameters, so the user's password is used as the original parameter for SHA256. However, our team found that the IV is still being derived from the password using SHA256 and is then converted to a Base64 string. While this approach is better than using a plaintext password as an IV, it is still not ideal. As noted in the synopsis of this Issue, it is generally recommended to use a randomly generated IV for each encryption operation, and to ensure that there is no overlap between the secret key and the IV (which can be public or safely stored in plaintext format).

This response is insufficient (updated 2023-09-11)

Issue D: Vulnerable and Unused Dependencies Detected in the Codebase

Fixed in https://github.com/Generative-Labs/Web3MQ-Snap/commit/994b8ed65166f70f3bcb5b8d2e7c6d450d831393

The rest of the findings are suggestions and do not need to be resolved.

Montoya commented 1 year ago

This Snap is pending various metadata fixes before it can be allowlisted.

Montoya commented 1 year ago

Our security team has reviewed Issue C from the list above and finds the response from the dev team insufficient. The IV needs to be random. The Snap will not be allowlisted at this time.

Montoya commented 1 year ago

Updated audit report from Least Authority: Least_Authority_Generative_Labs_MetaMask_Snap_Updated_Final_Audit.pdf

Montoya commented 1 year ago

Updated list of findings:

Issue A: Weak Key Derivation Algorithm Used

Fixed in https://github.com/Generative-Labs/Web3MQ-Snap/commit/f03fa7e48c26ede4ac3ff6dc09463acfed748a07

Issue B: Unnecessary Usage of Non-Standard Libraries

Statement from Least Authority:

The Generative Labs team responded that the npm package @noble/ed25519 is both recommended and user-friendly. While our team agrees with this statement, this Issue was identified in order to suggest alternatives for the usage of libraries, such as js-sha224 and js-sha3. Our team continues to recommend the usage of SHA256 instead of SHA3-224.

Response from MetaMask:

The js-sha3 package is well established. Upgrading to the built-in SubtleCrypto would be better, but is not required.

Issue C: Initialization Vector Used Does Not Meet Recommended Best Practices

Fixed in https://github.com/Generative-Labs/Web3MQ-Snap/pull/9

Issue D: Vulnerable and Unused Dependencies Detected in the Codebase

Fixed in https://github.com/Generative-Labs/Web3MQ-Snap/commit/994b8ed65166f70f3bcb5b8d2e7c6d450d831393

The rest of the findings are suggestions and do not need to be resolved.