Issue 4339 - Replace M2Crypto by PyCrypto

packagerSafari.py

 # 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
 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
@@ skipping 88 matching lines @@
             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):
     from Crypto.PublicKey import RSA
 
-    certs = []
     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)
-
-    for match in re.finditer(r'-+BEGIN CERTIFICATE-+(.*?)-+END CERTIFICATE-+', data, re.S):
-        certs.append(base64.b64decode(match.group(1)))
-
-    return certs, key
+
+    certificates = []
+    key = None
+    for match in re.finditer(r'-+BEGIN (.*?)-+(.*?)-+END \1-+', data, re.S):
+        section = match.group(1)
+        if section == 'CERTIFICATE':
+            certificates.append(base64.b64decode(match.group(2)))
+        elif section == 'PRIVATE KEY':
+            key = RSA.importKey(match.group(0))
+    if not key:
+        raise Exception('Could not find private key in file')
+
+    return certificates, key
+
+
+def _get_sequence(data):
+    from Crypto.Util import asn1
+    sequence = asn1.DerSequence()
+    sequence.decode(data)
+    return sequence
 
 
 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:
         # 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]
+        tbscertificate = _get_sequence(cert)[0]
+        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')
 
 
@@ skipping 48 matching lines @@
                 os.close(fd)
 
         # add certificates and placeholder signature
         # to the xar archive, and get data to sign
         fd, digest_filename = tempfile.mkstemp()
         os.close(fd)
         try:
             subprocess.check_call(
                 [
                     'xar', '--sign', '-f', outFile,
                     '--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(digest_filename, 'rb') as file:
                 digest = file.read()
         finally:
             os.unlink(digest_filename)
@@ skipping 64 matching lines @@
     fixAbsoluteUrls(files)
 
     dirname = metadata.get('general', 'basename') + '.safariextension'
     for filename in files.keys():
         files[os.path.join(dirname, filename)] = files.pop(filename)
 
     if not devenv and keyFile:
         createSignedXarArchive(outFile, files, certs, key)
     else:
         files.zip(outFile)