Context.decrypt() throws an error if *any* signature is bad
Open, NormalPublic

Description

the logic behind Context.decrypt()'s signature verification looks like it's going to throw an error if *any* signature does not validate.

However, the logic for most applications that pass verify=True is that they need *one* signature to validate -- the rest could be garbage, or signatures using new algorithms that we don't know anything about, or signatures from keys we don't know about yet.

I think the current implementation is brittle, and is prone to cause problems, and the logic probably needs an overhaul.

furthermore, if a list of keys is passed to verify=, the logic says that a signature from each key is needed. that's fine, but the current logic is that *even if that's the case*, an additional (spurious, broken, or noisy) signature will cause the decrypt operation to throw an error.

This function should be less brittle, and the test suite should exercise it (maybe by making someting that has two signatures: one of them a deliberately broken, but the other one valid).

dkg created this task.Dec 5 2018, 6:18 AM

Is this fixed now?

dkg added a comment.Dec 5 2018, 2:04 PM

@aheinecke thanks for the merge of my other branch! sadly, that branch does *not* address this issue yet. It doesn't even test for it. :( I can work on trying to fix it (and test it) if there's a consensus that we want this particular change in behavior.

dkg added a comment.Dec 5 2018, 6:30 PM

One more semantic question about how folks think Context.decrypt(verify=True) should work: if the decrypted thing has no signature at all, should the function succeed without throwing an exception? it currently does, but the returned verify_result has its signatures member set to the empty list.

I think this is correct ("i verified all signatures i found, and here they are (all none of them)"), but it might suprise someone who expects verify=True to throw an error if the data wasn't signed.

Anyway, if this is correct behavior (i think it is), then why should the existence of an invalid signature cause an exception to be thrown at all? If the bad signature was just stripped, no error would be thrown. I'm inclined to say that Context.decrypt() should just filter out (ignore) all bad signatures from its returned verify_result. That will avoid changing the current semantics too drastically -- the current function only ever returns lists of valid signatures -- while making it less brittle in the face of unevaluable signatures.

werner triaged this task as Normal priority.Mar 19 2019, 1:44 PM

@dkg If you propose a patch here I'm pretty sure that we will accept it. As one of our Python binding users you know better then us how the API should behave.

dkg added a comment.May 3 2019, 6:49 AM

I've just published a branch dkg/fix-T4276 (with commit 4100794e305ba22241ea5a4f7b42bb5189fbd948) which i think resolves this issue.

The earlier three patches in the series extend the test suite to make sure that it's doing the right thing

I'm for merging this as I understand the rationale. In Kleo / GpgOL I also only need one valid signature.

But I think as this is an API break, a software that currently relies on the exception might get a security problem if the exception is no longer raised. So I'm a bit cautions to make this change if we don't also bump the python gpgme major version.
Adding a different function like Context.decrypt_ex or something like that also seems bad though. :-/

What is your opinion on that?

dkg added a comment.May 3 2019, 5:23 PM

I agree that this is a minor API shift, but i *don't* think it's a security problem, because i was particularly careful to maintain the invariant that decrypt(verify=True) will only ever return valid signatures.

Can you describe the logic of a system that depends on the exception?

Note that any tool that depends on a BadSignatures exception is already vulnerable (in the currently-distributed version of python-gpg) to a malicious attacker that removes *all* signatures. in that case, no BadSignatures exception will be raised, and the verify_result will be empty. If you can point me to any downstream projects that depend on the BadSignatures exception, i'd be happy to try an attack and file a bug report if it's successful.

So i do not think that a change to the major version is necessary, and i think this can be merged without any additional security concerns.

Thanks for the explanation. That addresses my concerns.

aheinecke closed this task as Resolved.May 6 2019, 8:49 AM
aheinecke claimed this task.

Merged. Thanks again for your work on this.

dkg reopened this task as Open.Jul 8 2019, 4:38 AM
dkg added a subscriber: werner.

rM7d0a979c07d2 disabled the test for this. @werner says:

Feel free to fix it but a "make -j3 distcheck" MUST work.

It does work for me with rM7d0a979c07d2 reverted.

werner added a comment.Jul 8 2019, 9:53 AM

Using several python versions?

dkg added a comment.Jul 8 2019, 5:55 PM

yes, python2.7 and python3.7

dkg added a comment.Aug 27 2019, 4:18 PM

So i've been able to (intermittently) reproduce the failures that i think @werner was alluding to here, but not under any circumstances where i can get them to happen reliably to understand what's going on.

The error is (occasionally):

Traceback (most recent call last):
  File "./t-decrypt-verify.py", line 87, in <module>
    assert len(verify_result.signatures) == 1
AssertionError
FAIL: t-decrypt-verify.py

i'm looking for a way to better understand this failure, but currently it only happens every once in a while, and each run of make -j3 distcheck takes minutes to complete, which makes it tough to narrow down the problem.

dkg added a comment.Aug 27 2019, 4:50 PM

i'm actually running make -j3 check, since make -j3 distcheck has the problems described in T4688.