Skip to content
Merged
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
57 changes: 56 additions & 1 deletion sonic_installer/bootloader/aboot.py
Original file line number Diff line number Diff line change
Expand Up @@ -2,13 +2,17 @@
Bootloader implementation for Aboot used on Arista devices
"""

import base64
import collections
import os
import re
import subprocess
import zipfile

import click

from M2Crypto import X509
Copy link
Copy Markdown
Contributor

@qiluo-msft qiluo-msft Jun 29, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

M2Crypto [](start = 5, length = 8)

Add this new dependency in setup.py?
#Closed

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It will be add in build_debian.sh in sonic-buildimage.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Your statement is true. But by best practice, you need to add new dependency in setup.py


In reply to: 446767082 [](ancestors = 446767082)

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added.


from ..common import (
HOST_PATH,
IMAGE_DIR_PREFIX,
Expand All @@ -18,6 +22,12 @@
from .bootloader import Bootloader

_secureboot = None

# For the signature format, see: https://github.com/aristanetworks/swi-tools/tree/master/switools
SWI_SIG_FILE_NAME = 'swi-signature'
SWIX_SIG_FILE_NAME = 'swix-signature'
ISSUERCERT = 'IssuerCert'

def isSecureboot():
global _secureboot
if _secureboot is None:
Expand Down Expand Up @@ -114,11 +124,56 @@ def get_binary_image_version(self, image_path):
def verify_binary_image(self, image_path):
try:
subprocess.check_call(['/usr/bin/unzip', '-tq', image_path])
# TODO: secureboot check signature
if not self._verify_secureboot_image(image_path):
Copy link
Copy Markdown
Contributor

@qiluo-msft qiluo-msft Jun 29, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

self._verify_secureboot_image(image_path) [](start = 19, length = 41)

Just return, no need to use if-block #Closed

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed.

return False
except subprocess.CalledProcessError:
return False
return True

def _verify_secureboot_image(self, image_path):
if isSecureboot():
Copy link
Copy Markdown
Contributor

@qiluo-msft qiluo-msft Jun 29, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if isSecureboot(): [](start = 8, length = 18)

If not, should you return True? #Closed

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good catch, fixed.

current_image_path = self.get_current_image()
current_swi_path = self._swi_image_path(current_image_path).replace('flash:', HOST_PATH + '/')
cert = self.getCert(image_path)
current_cert = self.getCert(current_swi_path)
if not cert or not current_cert:
return False
# Verify the signing certificates are from the same issuer
return str(cert.get_issuer()) == str(current_cert.get_issuer())
Copy link
Copy Markdown
Contributor

@qiluo-msft qiluo-msft Jun 29, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess there is use case of change issuer in long run. Could you comment out this part of code? #Closed

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@qiluo-msft , we only need to verify the image has been signed, not need to verify the issuer, right?


@classmethod
def getCert(cls, swiFile):
try:
with zipfile.ZipFile(swiFile, 'r') as swi:
sigInfo = swi.getinfo(cls.getSigFileName(swiFile))
with swi.open(sigInfo, 'r') as sigFile:
for line in sigFile:
data = line.split(':')
if len(data) == 2:
if data[0] == ISSUERCERT:
cert = cls.base64Decode( data[1].strip() )
signingCert = X509.load_cert_string(cert)
return signingCert
else:
print( 'Unexpected format for line in swi[x]-signature file: %s' % line )
Copy link
Copy Markdown
Contributor

@qiluo-msft qiluo-msft Jun 29, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

print [](start = 28, length = 5)

print to stderr #Closed

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed.

return None
except KeyError:
Copy link
Copy Markdown
Contributor

@qiluo-msft qiluo-msft Jun 29, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

except KeyError: [](start = 8, length = 16)

if only one line of code could raise exception, could you minimize your try-block? #Closed

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed.

# Occurs if SIG_FILE_NAME is not in the swi (the SWI is not signed properly)
return None

@classmethod
def getSigFileName(cls, swiFile):
if swiFile.lower().endswith(".swix"):
return SWIX_SIG_FILE_NAME
return SWI_SIG_FILE_NAME

@classmethod
def base64Decode(cls, text):
try:
return base64.standard_b64decode(text)
except TypeError:
Copy link
Copy Markdown
Contributor

@qiluo-msft qiluo-msft Jun 29, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

TypeError [](start = 15, length = 9)

Is it better to catch in getCert() ? #Closed

return ""

@classmethod
def detect(cls):
with open('/proc/cmdline') as f:
Expand Down