Issue 4339 - Replace M2Crypto by PyCrypto

packagerSafari.py

Index: packagerSafari.py
===================================================================
--- a/packagerSafari.py
+++ b/packagerSafari.py
@@ -1,16 +1,18 @@
 # This Source Code Form is subject to the terms of the Mozilla Public
 # License, v. 2.0. If a copy of the MPL was not distributed with this
 # file, You can obtain one at http://mozilla.org/MPL/2.0/.
 
+import base64
+import ConfigParser
+import json
+import math
 import os
 import re
-import json
-import ConfigParser
 from urlparse import urlparse
 
 from packager import readMetadata, getDefaultFileName, getBuildVersion, getTemplate, Files
 from packagerChrome import convertJS, importGeckoLocales, getIgnoredFiles, getPackageFiles, defaultLocale, createScriptPage
 
 
 def processFile(path, data, params):
     return data
@@ -105,51 +107,67 @@ def fixAbsoluteUrls(files):
             files[filename] = re.sub(
                 r'(<[^<>]*?\b(?:href|src)\s*=\s*["\']?)\/+',
                 r'\1' + '/'.join(['..'] * filename.count('/') + ['']),
                 content, re.S | re.I
             )
 
 
 def get_certificates_and_key(keyfile):
-    import M2Crypto
+    from Crypto.PublicKey import RSA
 
     certs = []
-    bio = M2Crypto.BIO.openfile(keyfile)
+    with open(keyfile, 'r') as file:
+        data = file.read()
+    keydata = re.sub(r'(-+END PRIVATE KEY-+).*', r'\1', data, flags=re.S)

[Sebastian Noack, 2016/08/16 16:32:05]

Is this even necessary? I would expect RSA.importKey() to simply ignore
everything that is not a key in the file.

[Wladimir Palant, 2016/08/16 17:06:04]

Yes, I would expect that as well. However, I get "RSA key format not supported"
if I try it with the extra data.

[Sebastian Noack, 2016/08/17 08:43:07]

I just noticed that this code assumes the key to be at the beginning. This
wasn't case before.

[Wladimir Palant, 2016/08/17 10:03:15]

True, PyCrypto seems to be angry about certificates before the private key as
well. Fixed that.

+    key = RSA.importKey(keydata)
 
-    try:
-        key = M2Crypto.RSA.load_key_bio(bio)
-        bio.reset()
-        while True:
-            try:
-                certs.append(M2Crypto.X509.load_cert_bio(bio))
-            except M2Crypto.X509.X509Error:
-                break
-    finally:
-        bio.close()
+    for match in re.finditer(r'-+BEGIN CERTIFICATE-+(.*?)-+END CERTIFICATE-+', data, re.S):
+        certs.append(base64.b64decode(match.group(1)))
 
     return certs, key
 
 
 def get_developer_identifier(certs):
+    from Crypto.Util import asn1
+    def get_sequence(data):

[Sebastian Noack, 2016/08/16 16:32:05]

Any reason this is a nested function, and not just defined on the top-level?

[Wladimir Palant, 2016/08/16 17:06:04]

It isn't exactly a general-purpose function - merely a tiny helper for this
particular function.

[Sebastian Noack, 2016/08/17 08:43:07]

It doesn't need to be general-purpose to go on the top-level. Feel free to
prefix the function to mark it private. I generally think that nested functions,
make the code more complicated. And as per the Zen of Python, "flat is better
than nested".

[Wladimir Palant, 2016/08/17 10:03:15]

Done.

+        sequence = asn1.DerSequence()
+        sequence.decode(data)
+        return sequence
+
     for cert in certs:
-        subject = cert.get_subject()
-        for entry in subject.get_entries_by_nid(subject.nid['CN']):
-            m = re.match(r'Safari Developer: \((.*?)\)', entry.get_data().as_text())
-            if m:
-                return m.group(1)
+        # See https://tools.ietf.org/html/rfc5280#section-4
+        tbsCertificate = get_sequence(cert)[0]

[Sebastian Noack, 2016/08/16 16:32:05]

Nit: camel case

[Wladimir Palant, 2016/08/16 17:06:04]

I know. But that's how this field is named in the spec ;)

[Sebastian Noack, 2016/08/17 08:43:07]

That doesn't seem a reason to me, to violate PEP-8. The code is as
understandable if you just adapt it to underscore notation.

[Wladimir Palant, 2016/08/17 10:03:15]

Done.

+        subject = get_sequence(tbsCertificate)[5]
+
+        # We could decode the subject but since we have to apply a regular
+        # expression on CN entry anyway we can just skip that.
+        m = re.search(r'Safari Developer: \((\S*?)\)', subject)
+        if m:
+            return m.group(1)
 
     raise Exception('No Safari developer certificate found in chain')
 
 
+def sign_digest(key, digest):
+    from Crypto.Hash import SHA
+    from Crypto.Signature import PKCS1_v1_5
+
+    # xar already calculated the SHA1 digest so we have to fake hashing here.
+    class FakeHash(SHA.SHA1Hash):
+        def digest(self):
+            return digest
+
+    return PKCS1_v1_5.new(key).sign(FakeHash())
+
+
 def createSignedXarArchive(outFile, files, certs, key):
     import subprocess
     import tempfile
     import shutil
-    import M2Crypto
 
     # write files to temporary directory and create a xar archive
     dirname = tempfile.mkdtemp()
     try:
         for filename, contents in files.iteritems():
             path = os.path.join(dirname, filename)
 
             try:
@@ -170,51 +188,48 @@ def createSignedXarArchive(outFile, file
     certificate_filenames = []
     try:
         # write each certificate in DER format to a separate
         # temporary file, that they can be passed to xar
         for cert in certs:
             fd, filename = tempfile.mkstemp()
             try:
                 certificate_filenames.append(filename)
-                os.write(fd, cert.as_der())
+                os.write(fd, cert)
             finally:
                 os.close(fd)
 
         # add certificates and placeholder signature
         # to the xar archive, and get data to sign
-        fd, digestinfo_filename = tempfile.mkstemp()
+        fd, digest_filename = tempfile.mkstemp()
         os.close(fd)
         try:
             subprocess.check_call(
                 [
                     'xar', '--sign', '-f', outFile,
-                    '--digestinfo-to-sign', digestinfo_filename,
-                    '--sig-size', str(len(key.private_encrypt('', M2Crypto.RSA.pkcs1_padding)))
+                    '--data-to-sign', digest_filename,

[Wladimir Palant, 2016/08/16 15:12:08]

For reference: despite the misleading name, --data-to-sign won't actually write
the raw data to the file but a SHA1 digest of it. So it's essentially the same
thing as --digestinfo-to-sign, only that --digestinfo-to-sign will also
ASN.1-encode the digest. Getting ASN.1 encoding up front makes sense when
dealing with essentially broken RSA implementations like `openssl rsautl -sign`
but this seems to be the only signing library which will sign without doing
ASN.1 encoding (unlike the recommended `openssl pkeyutl -sign` for example). So
PyCrypto doesn't support the scenario where it would get an already encoded
digest, at least it could be convinced to accept an already pre-calculated SHA1
digest.

One would think that `xar --dump-toc-raw` would produce the data to be signed so
that tricks with pre-calculated hashes wouldn't be necessary. This seems to be
close to what's being hashed (in particular, xar definitely hashes compressed
data) but not quite it. In particular, the output produced here contains the toc
hash itself - so it seems to go too far.

[Wladimir Palant, 2016/08/16 15:37:38]

Actually, I finally realized what's going on there. `xar --dump-header` produces
the following output:

  size:                    28
  Compressed TOC length:   15572

That's the correct offset and length of the TOC, retrieving this block from the
file gives me the right hash. However, `xar --dump-toc-raw` produces a slice of
the file with the right length but starting at offset 64. Consequently, the data
is useless. This functionality isn't part of the original xar tool but has been
rather added by Kyle J. McKay - and it's simply buggy. I filed
https://github.com/mackyle/xar/issues/11 on this.

+                    '--sig-size', str(len(sign_digest(key, '')))
                 ] + [
                     arg for cert in certificate_filenames for arg in ('--cert-loc', cert)
                 ]
             )
 
-            with open(digestinfo_filename, 'rb') as file:
-                digestinfo = file.read()
+            with open(digest_filename, 'rb') as file:
+                digest = file.read()
         finally:
-            os.unlink(digestinfo_filename)
+            os.unlink(digest_filename)
     finally:
         for filename in certificate_filenames:
             os.unlink(filename)
 
     # sign data and inject signature into xar archive
     fd, signature_filename = tempfile.mkstemp()
     try:
         try:
-            os.write(fd, key.private_encrypt(
-                digestinfo,
-                M2Crypto.RSA.pkcs1_padding
-            ))
+            os.write(fd, sign_digest(key, digest))
         finally:
             os.close(fd)
 
         subprocess.check_call(['xar', '--inject-sig', signature_filename, '-f', outFile])
     finally:
         os.unlink(signature_filename)