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

gnupg_decrypt() does not throw error when a modified GPG message is handled #9

Closed
yahesh opened this issue Dec 21, 2016 · 5 comments
Closed

Comments

@yahesh
Copy link

yahesh commented Dec 21, 2016

We lately had to learn that gnupg_decrypt() does not seem to throw an error when the GPG message that is decrypted has been modified. This is especially true when content is appended to a valid GPG message.

To demonstrate the problem we created gnupg-pecl-test that implements a GPG decryption both as a direct call of the gpg binary as well as by leveraging the GnuPG PECL.

After configuring the FINGERPRINT, HOMEDIR and PASSPHRASE_FILE value we create an encrypted test file by calling:

# create file
echo "Test" > ./test

# encrypt file
gpg --recipient <test recipient> --encrypt ./test

So far, so good. Now we test the decryption of that file by calling the GPG binary directly, by using the GPG call feature and the PECL feature of gnupg-pecl-test.

# try to decrypt file
# returns: "Test"
gpg --quiet --no-tty --decrypt ./test.gpg

# get result code
# returns: "0"
echo $?

# try to decrypt file through execute script
# returns: "Test"
php ./gpg.php execute ./test.gpg

# try to decrypt file through PECL script
# returns: "Test"
php ./gpg.php pecl ./test.gpg

Now we modify the encrypted GPG message by appending data to it:

# modify file
echo "12345" >> ./test.gpg

And now we execute the decryption again:

# try to decrypt file
# returns: "Test\ngpg: WARNING: encrypted message has been manipulated!"
gpg --quiet --no-tty --decrypt ./test.gpg

# get result code
# returns: "2"
echo $?

# try to decrypt file through execute script
# returns: "ERROR: DECRYPTION FAILED"
php ./gpg.php execute ./test.gpg

# try to decrypt file through PECL script
# returns: "Test"
php ./gpg.php pecl ./test.gpg

The PECL feature checks whether the return value of gnupg_decrypt() is not FALSE and additionally checks that the return value of gnupg_geterror() is FALSE before returning the result of the decryption. However, neither return value indicates that a warning or error occurred during the decryption process.

In this case using the GnuPG PECL provides fewer information about the error state than calling the GPG binary directly.

The expected behaviour would be for gnupg_decrypt() to return FALSE or for gnupg_geterror() to return a value that is not FALSE.

@yahesh yahesh changed the title gnupg_decrypt() does not throw error when a modified GPG blob is handled gnupg_decrypt() does not throw error when a modified GPG message is handled Dec 21, 2016
@hannob
Copy link

hannob commented Jun 14, 2018

I saw this issue and if it'd be true then I'd say this is a security vulnerability, in the same class of vulns like efail.

However I can't follow what you're saying. In your example you're simply appending data to a message, which will cause gpg command line to give a warning about an invalid packet. While one can argue if it's best to then output the plaintext and discard invalid packets, I don't see a security risk with it.
When I modify the message itself with a hex editor (e.g. by flipping a bit in the last byte, which will break the MDC authentication tag) then gnupg_decrypt() outputs nothing. So it seems it correctly doesn't output data if the MDC fails.

This may be changed behavior as this bug was reported a while ago and I have newer versions of gpg/gpgme running. In any case it would be good to clarify if this is an unfixed security issue. Based on my observations I think it's not.

@yahesh
Copy link
Author

yahesh commented Jun 14, 2018

@hannob Dear Hanno, AFAIK this is still unresolved. The relevant part for me is that the gpg command line tool returns an error code != 0 in this case while gnupg PECL doesn't provide any information about the modified message.

In my use-case (which is the Shared-Secrets web app) this length extension attack is a real problem which has lead me to forcefully disabling the gnupg PECL support until the issue is fixed.

@hannob
Copy link

hannob commented Jun 14, 2018

Can you explain what the attack scenario here would be?
Maybe I'm missing something, but so far as I can see the only impact is that you can add extra data to a message that will be ignored. I see that one can think this is undesired behavior, but it looks harmless to me. I'd like to understand if it's relevant from a security perspective.

@yahesh
Copy link
Author

yahesh commented Jun 14, 2018

@hannob Shared-Secrets is a web app that lets you share confidential content by generating a link that contains the GPG encrypted representation of the shared content. When calling the URL the server decrypts the content and presents it to the user. To prevent such a URL from leaking the content to other parties it's only allowed to decrypt a URL once. To prevent a URL from being called more than once, the hash of the URL is stored in a database.

With the help of the unnoticed length extension it was possibly to retrieve a URL more than once by simply appending junk data to it - an attack that wasn't possible with the GPG command line tool but was introduced when optionally supporting the gnupg PECL.

That's why we gradually deprecated the support for the gnupg PECL (as can be seen in the corresponding CHANGELOG).

@yahesh
Copy link
Author

yahesh commented Dec 5, 2019

I retested this again and the problem seems to be solved in newer versions.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants