From 31a4ba72646de34d26923e2293a1ea08f52e9f7f Mon Sep 17 00:00:00 2001 From: Gilles Peskine Date: Mon, 17 Jun 2019 15:01:08 +0200 Subject: [PATCH 1/9] Fix misuse of signed ints in the HAVEGE module The elements of the HAVEGE state are manipulated with bitwise operations, with the expectations that the elements are 32-bit unsigned integers (or larger). But they are declared as int, and so the code has undefined behavior. Clang with Asan correctly points out some shifts that reach the sign bit. Use unsigned int internally. This is technically an aliasing violation since we're accessing an array of `int` via a pointer to `unsigned int`, but since we don't access the array directly inside the same function, it's very unlikely to be compiled in an unintended manner. --- library/havege.c | 19 ++++++++++--------- 1 file changed, 10 insertions(+), 9 deletions(-) diff --git a/library/havege.c b/library/havege.c index 54f897c6e..08f9d974d 100644 --- a/library/havege.c +++ b/library/havege.c @@ -54,7 +54,7 @@ * ------------------------------------------------------------------------ */ -#define SWAP(X,Y) { int *T = (X); (X) = (Y); (Y) = T; } +#define SWAP(X,Y) { unsigned *T = (X); (X) = (Y); (Y) = T; } #define TST1_ENTER if( PTEST & 1 ) { PTEST ^= 3; PTEST >>= 1; #define TST2_ENTER if( PTEST & 1 ) { PTEST ^= 3; PTEST >>= 1; @@ -77,7 +77,7 @@ PTX = (PT1 >> 18) & 7; \ PT1 &= 0x1FFF; \ PT2 &= 0x1FFF; \ - CLK = (int) mbedtls_timing_hardclock(); \ + CLK = (unsigned) mbedtls_timing_hardclock(); \ \ i = 0; \ A = &WALK[PT1 ]; RES[i++] ^= *A; \ @@ -100,7 +100,7 @@ \ IN = (*A >> (5)) ^ (*A << (27)) ^ CLK; \ *A = (*B >> (6)) ^ (*B << (26)) ^ CLK; \ - *B = IN; CLK = (int) mbedtls_timing_hardclock(); \ + *B = IN; CLK = (unsigned) mbedtls_timing_hardclock(); \ *C = (*C >> (7)) ^ (*C << (25)) ^ CLK; \ *D = (*D >> (8)) ^ (*D << (24)) ^ CLK; \ \ @@ -151,19 +151,20 @@ PT1 ^= (PT2 ^ 0x10) & 0x10; \ \ for( n++, i = 0; i < 16; i++ ) \ - hs->pool[n % MBEDTLS_HAVEGE_COLLECT_SIZE] ^= RES[i]; + POOL[n % MBEDTLS_HAVEGE_COLLECT_SIZE] ^= RES[i]; /* * Entropy gathering function */ static void havege_fill( mbedtls_havege_state *hs ) { - int i, n = 0; - int U1, U2, *A, *B, *C, *D; - int PT1, PT2, *WALK, RES[16]; - int PTX, PTY, CLK, PTEST, IN; + unsigned i, n = 0; + unsigned U1, U2, *A, *B, *C, *D; + unsigned PT1, PT2, *WALK, *POOL, RES[16]; + unsigned PTX, PTY, CLK, PTEST, IN; - WALK = hs->WALK; + WALK = (unsigned *) hs->WALK; + POOL = (unsigned *) hs->pool; PT1 = hs->PT1; PT2 = hs->PT2; From 04659a023ef8ffefffd0aab273a396b5d876ebf6 Mon Sep 17 00:00:00 2001 From: Gilles Peskine Date: Mon, 17 Jun 2019 15:12:51 +0200 Subject: [PATCH 2/9] Prevent building the HAVEGE module on platforms where it doesn't work If int is not capable of storing as many values as unsigned, the code may generate a trap value. If signed int and unsigned int aren't 32-bit types, the code may calculate meaningless values. --- library/havege.c | 11 +++++++++++ 1 file changed, 11 insertions(+) diff --git a/library/havege.c b/library/havege.c index 08f9d974d..c139e1db0 100644 --- a/library/havege.c +++ b/library/havege.c @@ -38,8 +38,19 @@ #include "mbedtls/timing.h" #include "mbedtls/platform_util.h" +#include #include +/* If int isn't capable of storing 2^32 distinct values, the code of this + * module may cause a processor trap or a miscalculation. If int is more + * than 32 bits, the code may not calculate the intended values. */ +#if INT_MIN + 1 != -0x7fffffff +#error "The HAVEGE module requires int to be exactly 32 bits, with INT_MIN = -2^31." +#endif +#if UINT_MAX != 0xffffffff +#error "The HAVEGE module requires unsigned to be exactly 32 bits." +#endif + /* ------------------------------------------------------------------------ * On average, one iteration accesses two 8-word blocks in the havege WALK * table, and generates 16 words in the RES array. From e43e3addbbc7dc39e798f2b49d5ab4bb454f61ba Mon Sep 17 00:00:00 2001 From: Gilles Peskine Date: Mon, 17 Jun 2019 15:15:40 +0200 Subject: [PATCH 3/9] Changelog entry for HAVEGE fix --- ChangeLog | 1 + 1 file changed, 1 insertion(+) diff --git a/ChangeLog b/ChangeLog index cec77d2af..ae78455d4 100644 --- a/ChangeLog +++ b/ChangeLog @@ -46,6 +46,7 @@ Bugfix * Fix multiple X.509 functions previously returning ASN.1 low-level error codes to always wrap these codes into X.509 high level error codes before returning. Fixes #2431. + * Fix misuse of signed arithmetic in the HAVEGE module. #2598 Changes * Return from various debugging routines immediately if the From 79cfef02d92aa322875f9aa610e0e1272fc02f70 Mon Sep 17 00:00:00 2001 From: Gilles Peskine Date: Thu, 4 Jul 2019 19:31:02 +0200 Subject: [PATCH 4/9] Use the docstring in the command line help --- tests/scripts/check-files.py | 20 +++++--------------- 1 file changed, 5 insertions(+), 15 deletions(-) diff --git a/tests/scripts/check-files.py b/tests/scripts/check-files.py index 00fd0edfb..a658415dc 100755 --- a/tests/scripts/check-files.py +++ b/tests/scripts/check-files.py @@ -1,11 +1,9 @@ #!/usr/bin/env python3 + +# This file is part of Mbed TLS (https://tls.mbed.org) +# Copyright (c) 2018, Arm Limited, All Rights Reserved + """ -This file is part of Mbed TLS (https://tls.mbed.org) - -Copyright (c) 2018, Arm Limited, All Rights Reserved - -Purpose - This script checks the current state of the source code for minor issues, including incorrect file permissions, presence of tabs, non-Unix line endings, trailing whitespace, presence of UTF-8 BOM, and TODO comments. @@ -257,15 +255,7 @@ class IntegrityChecker(object): def run_main(): - parser = argparse.ArgumentParser( - description=( - "This script checks the current state of the source code for " - "minor issues, including incorrect file permissions, " - "presence of tabs, non-Unix line endings, trailing whitespace, " - "presence of UTF-8 BOM, and TODO comments. " - "Note: requires python 3, must be run from Mbed TLS root." - ) - ) + parser = argparse.ArgumentParser(description=__doc__) parser.add_argument( "-l", "--log_file", type=str, help="path to optional output log", ) From 47d7c2d7cb4d6fb813751e3c1fcedac22730545b Mon Sep 17 00:00:00 2001 From: Gilles Peskine Date: Thu, 4 Jul 2019 19:31:33 +0200 Subject: [PATCH 5/9] Allow TODO in code Don't reject TODO in code. Fix #2587 --- tests/scripts/check-files.py | 16 +--------------- 1 file changed, 1 insertion(+), 15 deletions(-) diff --git a/tests/scripts/check-files.py b/tests/scripts/check-files.py index a658415dc..255bed8b9 100755 --- a/tests/scripts/check-files.py +++ b/tests/scripts/check-files.py @@ -6,7 +6,7 @@ """ This script checks the current state of the source code for minor issues, including incorrect file permissions, presence of tabs, non-Unix line endings, -trailing whitespace, presence of UTF-8 BOM, and TODO comments. +trailing whitespace, and presence of UTF-8 BOM. Note: requires python 3, must be run from Mbed TLS root. """ @@ -168,19 +168,6 @@ class MergeArtifactIssueTracker(LineIssueTracker): return True return False -class TodoIssueTracker(LineIssueTracker): - """Track lines containing ``TODO``.""" - - heading = "TODO present:" - files_exemptions = frozenset([ - os.path.basename(__file__), - "benchmark.c", - "pull_request_template.md", - ]) - - def issue_with_line(self, line, _filepath): - return b"todo" in line.lower() - class IntegrityChecker(object): """Sanity-check files under the current directory.""" @@ -209,7 +196,6 @@ class IntegrityChecker(object): TrailingWhitespaceIssueTracker(), TabIssueTracker(), MergeArtifactIssueTracker(), - TodoIssueTracker(), ] @staticmethod From c53b93b8eff8c767a857b2497cf00423b96ba527 Mon Sep 17 00:00:00 2001 From: Gilles Peskine Date: Thu, 4 Jul 2019 18:59:36 +0200 Subject: [PATCH 6/9] Allow running /somewhere/else/path/to/abi_check.py Don't require abi_check.py to be the one in scripts/ under the current directory. --- scripts/abi_check.py | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/scripts/abi_check.py b/scripts/abi_check.py index 502c7ae02..6edc54fde 100755 --- a/scripts/abi_check.py +++ b/scripts/abi_check.py @@ -59,9 +59,7 @@ class AbiChecker(object): @staticmethod def check_repo_path(): - current_dir = os.path.realpath('.') - root_dir = os.path.dirname(os.path.dirname(os.path.realpath(__file__))) - if current_dir != root_dir: + if not all(os.path.isdir(d) for d in ["include", "library", "tests"]): raise Exception("Must be run from Mbed TLS root") def _setup_logger(self): From 826286a04ed3785ad925051a960c574be506166a Mon Sep 17 00:00:00 2001 From: Gilles Peskine Date: Thu, 4 Jul 2019 19:00:31 +0200 Subject: [PATCH 7/9] Document how to build the typical argument for -s --- scripts/abi_check.py | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/scripts/abi_check.py b/scripts/abi_check.py index 6edc54fde..e8ab1cb79 100755 --- a/scripts/abi_check.py +++ b/scripts/abi_check.py @@ -355,7 +355,9 @@ def run_main(): ) parser.add_argument( "-s", "--skip-file", type=str, - help="path to file containing symbols and types to skip" + help=("path to file containing symbols and types to skip " + "(typically \"-s identifiers\" after running " + "\"tests/scripts/list-identifiers.sh --internal\")") ) parser.add_argument( "-b", "--brief", action="store_true", From ea94391500b8104e804bac13d69d4d4c2369c757 Mon Sep 17 00:00:00 2001 From: Gilles Peskine Date: Thu, 4 Jul 2019 19:01:22 +0200 Subject: [PATCH 8/9] Record the commits that were compared Record the commit ID in addition to the symbolic name of the version being tested. This makes it easier to figure out what has been compared when reading logs that don't always indicate explicitly what things like HEAD are. This makes the title of HTML reports somewhat verbose, but I think that's a small price to pay. --- scripts/abi_check.py | 22 ++++++++++++++++++++-- 1 file changed, 20 insertions(+), 2 deletions(-) diff --git a/scripts/abi_check.py b/scripts/abi_check.py index e8ab1cb79..6aef9924f 100755 --- a/scripts/abi_check.py +++ b/scripts/abi_check.py @@ -106,6 +106,12 @@ class AbiChecker(object): stderr=subprocess.STDOUT ) self.log.debug(worktree_output.decode("utf-8")) + version.commit = subprocess.check_output( + [self.git_command, "rev-parse", worktree_rev], + cwd=git_worktree_path, + stderr=subprocess.STDOUT + ).decode("ascii").rstrip() + self.log.debug("Commit is {}".format(version.commit)) return git_worktree_path def _update_git_submodules(self, git_worktree_path, version): @@ -161,6 +167,13 @@ class AbiChecker(object): os.path.join(root, file) ) + @staticmethod + def _pretty_revision(version): + if version.revision == version.commit: + return version.revision + else: + return "{} ({})".format(version.revision, version.commit) + def _get_abi_dumps_from_shared_libraries(self, version): """Generate the ABI dumps for the specified git revision. The shared libraries must have been built and the module paths @@ -175,7 +188,7 @@ class AbiChecker(object): "abi-dumper", module_path, "-o", output_path, - "-lver", version.revision + "-lver", self._pretty_revision(version), ] abi_dump_output = subprocess.check_output( abi_dump_command, @@ -224,7 +237,10 @@ class AbiChecker(object): """Generate a report of the differences between the reference ABI and the new ABI. ABI dumps from self.old_version and self.new_version must be available.""" - compatibility_report = "" + compatibility_report = ("Checking evolution from {} to {}\n".format( + self._pretty_revision(self.old_version), + self._pretty_revision(self.new_version) + )) compliance_return_code = 0 shared_modules = list(set(self.old_version.modules.keys()) & set(self.new_version.modules.keys())) @@ -371,6 +387,7 @@ def run_main(): version="old", repository=abi_args.old_repo, revision=abi_args.old_rev, + commit=None, crypto_repository=abi_args.old_crypto_repo, crypto_revision=abi_args.old_crypto_rev, abi_dumps={}, @@ -380,6 +397,7 @@ def run_main(): version="new", repository=abi_args.new_repo, revision=abi_args.new_rev, + commit=None, crypto_repository=abi_args.new_crypto_repo, crypto_revision=abi_args.new_crypto_rev, abi_dumps={}, From 47a4cba1db2526fba40ffcc66e862a50e0ffe01b Mon Sep 17 00:00:00 2001 From: Gilles Peskine Date: Thu, 4 Jul 2019 19:17:40 +0200 Subject: [PATCH 9/9] Split _abi_compliance_command into smaller functions This makes the code easier to read and pacifies pylint. --- scripts/abi_check.py | 115 ++++++++++++++++++++++++------------------- 1 file changed, 63 insertions(+), 52 deletions(-) diff --git a/scripts/abi_check.py b/scripts/abi_check.py index 6aef9924f..533aaea3d 100755 --- a/scripts/abi_check.py +++ b/scripts/abi_check.py @@ -233,73 +233,84 @@ class AbiChecker(object): if not problems.getchildren(): report.remove(problems) + def _abi_compliance_command(self, mbed_module, output_path): + """Build the command to run to analyze the library mbed_module. + The report will be placed in output_path.""" + abi_compliance_command = [ + "abi-compliance-checker", + "-l", mbed_module, + "-old", self.old_version.abi_dumps[mbed_module], + "-new", self.new_version.abi_dumps[mbed_module], + "-strict", + "-report-path", output_path, + ] + if self.skip_file: + abi_compliance_command += ["-skip-symbols", self.skip_file, + "-skip-types", self.skip_file] + if self.brief: + abi_compliance_command += ["-report-format", "xml", + "-stdout"] + return abi_compliance_command + + def _is_library_compatible(self, mbed_module, compatibility_report): + """Test if the library mbed_module has remained compatible. + Append a message regarding compatibility to compatibility_report.""" + output_path = os.path.join( + self.report_dir, "{}-{}-{}.html".format( + mbed_module, self.old_version.revision, + self.new_version.revision + ) + ) + try: + subprocess.check_output( + self._abi_compliance_command(mbed_module, output_path), + stderr=subprocess.STDOUT + ) + except subprocess.CalledProcessError as err: + if err.returncode != 1: + raise err + if self.brief: + self.log.info( + "Compatibility issues found for {}".format(mbed_module) + ) + report_root = ET.fromstring(err.output.decode("utf-8")) + self._remove_extra_detail_from_report(report_root) + self.log.info(ET.tostring(report_root).decode("utf-8")) + else: + self.can_remove_report_dir = False + compatibility_report.append( + "Compatibility issues found for {}, " + "for details see {}".format(mbed_module, output_path) + ) + return False + compatibility_report.append( + "No compatibility issues for {}".format(mbed_module) + ) + if not (self.keep_all_reports or self.brief): + os.remove(output_path) + return True + def get_abi_compatibility_report(self): """Generate a report of the differences between the reference ABI and the new ABI. ABI dumps from self.old_version and self.new_version must be available.""" - compatibility_report = ("Checking evolution from {} to {}\n".format( + compatibility_report = ["Checking evolution from {} to {}".format( self._pretty_revision(self.old_version), self._pretty_revision(self.new_version) - )) + )] compliance_return_code = 0 shared_modules = list(set(self.old_version.modules.keys()) & set(self.new_version.modules.keys())) for mbed_module in shared_modules: - output_path = os.path.join( - self.report_dir, "{}-{}-{}.html".format( - mbed_module, self.old_version.revision, - self.new_version.revision - ) - ) - abi_compliance_command = [ - "abi-compliance-checker", - "-l", mbed_module, - "-old", self.old_version.abi_dumps[mbed_module], - "-new", self.new_version.abi_dumps[mbed_module], - "-strict", - "-report-path", output_path, - ] - if self.skip_file: - abi_compliance_command += ["-skip-symbols", self.skip_file, - "-skip-types", self.skip_file] - if self.brief: - abi_compliance_command += ["-report-format", "xml", - "-stdout"] - try: - subprocess.check_output( - abi_compliance_command, - stderr=subprocess.STDOUT - ) - except subprocess.CalledProcessError as err: - if err.returncode == 1: - compliance_return_code = 1 - if self.brief: - self.log.info( - "Compatibility issues found for {}".format(mbed_module) - ) - report_root = ET.fromstring(err.output.decode("utf-8")) - self._remove_extra_detail_from_report(report_root) - self.log.info(ET.tostring(report_root).decode("utf-8")) - else: - self.can_remove_report_dir = False - compatibility_report += ( - "Compatibility issues found for {}, " - "for details see {}\n".format(mbed_module, output_path) - ) - else: - raise err - else: - compatibility_report += ( - "No compatibility issues for {}\n".format(mbed_module) - ) - if not (self.keep_all_reports or self.brief): - os.remove(output_path) + if not self._is_library_compatible(mbed_module, + compatibility_report): + compliance_return_code = 1 for version in [self.old_version, self.new_version]: for mbed_module, mbed_module_dump in version.abi_dumps.items(): os.remove(mbed_module_dump) if self.can_remove_report_dir: os.rmdir(self.report_dir) - self.log.info(compatibility_report) + self.log.info("\n".join(compatibility_report)) return compliance_return_code def check_for_abi_changes(self):