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

ed25519 keys crash on parse #222

Closed
anarcat opened this issue Nov 8, 2017 · 1 comment
Closed

ed25519 keys crash on parse #222

anarcat opened this issue Nov 8, 2017 · 1 comment
Assignees
Labels
Milestone

Comments

@anarcat
Copy link
Contributor

anarcat commented Nov 8, 2017

While testing #221, i have found that I can reliably crash the parsing library on a public key that uses ed25519 elliptic curves.

With this test program:

pgpy.PGPKey.from_file('micah.asc')

... and micah's public key (ed25519-micah.asc.txt, Micah Anderson <micah@riseup.net> on the keyservers), I get the following backtrace:

In [77]: pgpy.PGPKey.from_file('micah.asc')
---------------------------------------------------------------------------
AttributeError                            Traceback (most recent call last)
/home/anarcat/src/PGPy/pgpy/types.py in __call__(cls, packet)
    559             try:
--> 560                 obj.parse(packet)
    561 

/home/anarcat/src/PGPy/pgpy/packet/packets.py in parse(self, packet)
    448 
--> 449         self.signature.parse(packet)
    450 

AttributeError: 'NoneType' object has no attribute 'parse'

The above exception was the direct cause of the following exception:

PGPError                                  Traceback (most recent call last)
<ipython-input-77-327727931a7e> in <module>()
----> 1 pgpy.PGPKey.from_file('micah.asc')

/home/anarcat/src/PGPy/pgpy/types.py in from_file(cls, filename)
    190             file.readinto(data)
    191 
--> 192         po = obj.parse(data)
    193 
    194         if po is not None:

/home/anarcat/src/PGPy/pgpy/pgp.py in parse(self, data)
   2298 
   2299                 # add signatures to whatever we got
-> 2300                 [ operator.ior(pgpobj, PGPSignature() | sig) for sig in group if not isinstance(sig, Opaque) ]
   2301 
   2302                 # and file away pgpobj

/home/anarcat/src/PGPy/pgpy/pgp.py in <listcomp>(.0)
   2298 
   2299                 # add signatures to whatever we got
-> 2300                 [ operator.ior(pgpobj, PGPSignature() | sig) for sig in group if not isinstance(sig, Opaque) ]
   2301 
   2302                 # and file away pgpobj

/home/anarcat/src/PGPy/pgpy/pgp.py in <lambda>(d)
   2267 
   2268         ##TODO: see issue #141 and fix this better
-> 2269         getpkt = lambda d: Packet(d) if len(d) > 0 else None  # flake8: noqa
   2270         # some packets are filtered out
   2271         getpkt = filter(lambda p: p.header.tag != PacketTag.Trust, iter(functools.partial(getpkt, data), None))

/home/anarcat/src/PGPy/pgpy/types.py in __call__(cls, packet)
    561 
    562             except Exception as ex:
--> 563                 six.raise_from(PGPError(str(ex)), ex)
    564 
    565         else:

/usr/local/lib/python3.5/dist-packages/six.py in raise_from(value, from_value)

PGPError: 'NoneType' object has no attribute 'parse'
@Commod0re Commod0re self-assigned this Nov 8, 2017
@Commod0re Commod0re added the bug label Nov 8, 2017
@Commod0re
Copy link
Contributor

found the issue, the signature is not parsing correctly because of this bit of code in class SignatureV4 (packet/packets.py):

    @pubalg.register(int)
    @pubalg.register(PubKeyAlgorithm)
    def pubalg_int(self, val):
        self._pubalg = PubKeyAlgorithm(val)

        sigs = {PubKeyAlgorithm.RSAEncryptOrSign: RSASignature,
                PubKeyAlgorithm.RSAEncrypt: RSASignature,
                PubKeyAlgorithm.RSASign: RSASignature,
                PubKeyAlgorithm.DSA: DSASignature,
                PubKeyAlgorithm.ECDSA: ECDSASignature, }

        if self.pubalg in sigs:
            self.signature = sigs[self.pubalg]()

The key provided above specifies the pubkey algorithm as 0x16, which is defined as PubKeyAlgorithm.EdDSA but no signature field object is instantiated. So two things should happen:

  1. PubKeyAlgorithm.EdDSA should be added to that dict (and EdDSASignature may also need to be defined if the format differs from ECDSASignature) so that it can actually parse

  2. an OpaqueSignature class should be added to pgpy/packet/fields.py and used as a fallback, there's no reason for PGPy to raise an exception here.

I plan to implement this tonight and add appropriate test material to the test suite, and then get this and #217 out the door as PGPy v0.4.4

@Commod0re Commod0re added this to the 0.4.4 milestone Nov 8, 2017
Commod0re added a commit that referenced this issue Nov 8, 2017
dkg pushed a commit to dkg/PGPy that referenced this issue Aug 1, 2019
This increases the versioned dependency on the cryptography module to
2.6, since that is the version that provides the necessary ed25519
functionality.

We also add a "pure" 25519 OpenPGP certificate for testing purposes.

Closes SecurityInnovation#221, SecurityInnovation#222, SecurityInnovation#247

Signed-off-by: Daniel Kahn Gillmor <dkg@fifthhorseman.net>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants