Skip to content
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

Change return type Decrypter.Decrypt #532

Open
robdefeo opened this issue Feb 2, 2020 · 3 comments
Open

Change return type Decrypter.Decrypt #532

robdefeo opened this issue Feb 2, 2020 · 3 comments
Labels
area/crypto enhancement New feature or request help wanted Extra attention is needed

Comments

@robdefeo
Copy link
Member

robdefeo commented Feb 2, 2020

Is your feature request related to a problem? Please describe.
To reduce the attack options, returning generic error messages is prefered. The method signature in crypto/cipher/cipher.go of Decrypter.Decrypt(EncryptedContent) (PlainContent, error) should return ErrDecrypt for any error. This is confusing, the signature suggests a descriptive error message will be returned on failure.

I don't think it's a good idea to mask all errors with the same error. It's okay to return the same error type, but it should not be the same error message for different types of errors, the actual error is getting masked here. It'll be harder to debug the reason just by seeing "error in decryption".

Originally posted by @hasitpbhatt in #509

Describe the solution you'd like
There appears to be 3 approaches:

  • Return detailed error messages for the failure:
    • Good: easier to debug
    • Bad: exposes error message to potential attacker.
  • Return set error messages from crypto/cipher/errors.go
    • Good: Error message can not be used to target attacks
    • Bad: More difficult to debug, unclear from signature that only one error will be returned
  • Change signature to Decrypt(EncryptedContent) (PlainContent, error) as in https://godoc.org/golang.org/x/crypto/nacl/secretbox#Open
    • Good: Error message can not be used to target attacks
    • Bad: More difficult to debug
@robdefeo robdefeo added enhancement New feature or request help wanted Extra attention is needed area/crypto labels Feb 2, 2020
@robdefeo
Copy link
Member Author

robdefeo commented Feb 2, 2020

I want to have some robust discussion on this issue before starting it.

I am learning to changing the signature as its clear.

@developerfred
Copy link
Contributor

@robdefeo We can make a definition of error only of the Mailchain, error xXXX, linking the code to a Debug manual. However, it is only available in the developer slack.

@robdefeo
Copy link
Member Author

robdefeo commented Feb 6, 2020

@developerfred logging the output is an option the bridges both the options. This gives the information to a potential attacked easily though.

I am aware an attacker could download the codebase make changes to the code to log or provide information. I think we should be making efforts to make this difficult.

@stale stale bot added the Stale label Jun 5, 2020
@robdefeo robdefeo removed the Stale label Jun 6, 2020
@mailchain mailchain deleted a comment from stale bot Jun 6, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/crypto enhancement New feature or request help wanted Extra attention is needed
Projects
None yet
Development

No branches or pull requests

2 participants