-
Notifications
You must be signed in to change notification settings - Fork 2.4k
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
Enable New CodeQL Queries - CWE 120, 457, 676, 758, 787, 805 #4133
Closed
makubacki
wants to merge
12
commits into
tianocore:master
from
makubacki:enable_codeql_condunit_pointer_buffer_overflow_v4
Closed
Enable New CodeQL Queries - CWE 120, 457, 676, 758, 787, 805 #4133
makubacki
wants to merge
12
commits into
tianocore:master
from
makubacki:enable_codeql_condunit_pointer_buffer_overflow_v4
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
makubacki
force-pushed
the
enable_codeql_condunit_pointer_buffer_overflow_v4
branch
from
March 10, 2023 15:16
2c17f41
to
c71d38a
Compare
Details for these CodeQL alerts can be found here: - Pointer overflow check (cpp/pointer-overflow-check): - https://cwe.mitre.org/data/definitions/758.html - Potential buffer overflow check (cpp/potential-buffer-overflow): - https://cwe.mitre.org/data/definitions/676.html CodeQL alert: - Line 1612 in MdeModulePkg/Universal/SmbiosDxe/SmbiosDxe.c - Type: Pointer overflow check - Severity: Low - Problem: Range check relying on pointer overflow Cc: Dandan Bi <dandan.bi@intel.com> Cc: Erich McMillan <emcmillan@microsoft.com> Cc: Jian J Wang <jian.j.wang@intel.com> Cc: Liming Gao <gaoliming@byosoft.com.cn> Cc: Michael Kubacki <mikuback@linux.microsoft.com> Cc: Star Zeng <star.zeng@intel.com> Cc: Zhichao Gao <zhichao.gao@intel.com> Cc: Zhiguang Liu <zhiguang.liu@intel.com> Co-authored-by: Michael Kubacki <michael.kubacki@microsoft.com> Signed-off-by: Erich McMillan <emcmillan@microsoft.com> Reviewed-by: Liming Gao <gaoliming@byosoft.com.cn>
Purdue Compiler Construction Tool Set (PCCTS) source code was copied/ pasted into BaseTools/Source/C/VfrCompile/Pccts/. The code contains tab characters instead of spaces. PatchCheck.py gives an error on modifications to files that contain tabs. This change adds that directory to the pre-existing list of directories in which tab checks are ignored in PatchCheck.py. The goal of my upcoming change there is not to mix tabs and spaces but to fix a bug while preserving its current formatting characters. Cc: Bob Feng <bob.c.feng@intel.com> Cc: Liming Gao <gaoliming@byosoft.com.cn> Cc: Michael D Kinney <michael.d.kinney@intel.com> Cc: Sean Brogan <sean.brogan@microsoft.com> Cc: Yuwei Chen <yuwei.chen@intel.com> Signed-off-by: Michael Kubacki <michael.kubacki@microsoft.com> Reviewed-by: Liming Gao <gaoliming@byosoft.com.cn> Reviewed-by: Michael D Kinney <michael.d.kinney@intel.com>
While more portable methods exist to handle these cases, this change does not attempt to do more than fix the immediate problem and follow the conventions already established in this code. `snprintf()` is introduced as the minimum improvement apart from making the buffers larger. Fixes the following CodeQL alerts: 1. Failure on line 2339 in BaseTools/Source/C/VfrCompile/Pccts/antlr/gen.c - Type: Potentially overrunning write - Severity: Critical - Problem: This 'call to sprintf' operation requires 17 bytes but the destination is only 16 bytes. 2. Failure on line 2341 in BaseTools/Source/C/VfrCompile/Pccts/antlr/gen.c - Type: Potentially overrunning write - Severity: Critical - Problem: This 'call to sprintf' operation requires 17 bytes but the destination is only 16 bytes. 3. Failure on line 1309 in BaseTools/Source/C/VfrCompile/Pccts/antlr/main.c - Type: Potentially overrunning write - Severity: Critical - Problem: This 'call to sprintf' operation requires 25 bytes but the destination is only 20 bytes. Cc: Bob Feng <bob.c.feng@intel.com> Cc: Liming Gao <gaoliming@byosoft.com.cn> Cc: Michael D Kinney <michael.d.kinney@intel.com> Cc: Sean Brogan <sean.brogan@microsoft.com> Cc: Yuwei Chen <yuwei.chen@intel.com> Signed-off-by: Michael Kubacki <michael.kubacki@microsoft.com> Reviewed-by: Liming Gao <gaoliming@byosoft.com.cn> Reviewed-by: Michael D Kinney <michael.d.kinney@intel.com>
Fixes CodeQL alerts for CWE-457: https://cwe.mitre.org/data/definitions/457.html Checks the return value from `ASN1_get_object()` to verify values set by the function are valid. Note that the function returns literal `0x80`: `return (0x80);` That is used to check the return value is as the case in other areas of the code. Cc: Erich McMillan <emcmillan@microsoft.com> Cc: Guomin Jiang <guomin.jiang@intel.com> Cc: Jian J Wang <jian.j.wang@intel.com> Cc: Jiewen Yao <jiewen.yao@intel.com> Cc: Michael Kubacki <mikuback@linux.microsoft.com> Cc: Xiaoyu Lu <xiaoyu1.lu@intel.com> Co-authored-by: Erich McMillan <emcmillan@microsoft.com> Signed-off-by: Michael Kubacki <michael.kubacki@microsoft.com> Reviewed-by: Jiewen Yao <Jiewen.yao@intel.com>
Fixes CodeQL alerts for CWE-457: https://cwe.mitre.org/data/definitions/457.html Cc: Dandan Bi <dandan.bi@intel.com> Cc: Eric Dong <eric.dong@intel.com> Cc: Erich McMillan <emcmillan@microsoft.com> Cc: Guomin Jiang <guomin.jiang@intel.com> Cc: Jian J Wang <jian.j.wang@intel.com> Cc: Liming Gao <gaoliming@byosoft.com.cn> Cc: Michael Kubacki <mikuback@linux.microsoft.com> Cc: Ray Ni <ray.ni@intel.com> Cc: Zhichao Gao <zhichao.gao@intel.com> Co-authored-by: Erich McMillan <emcmillan@microsoft.com> Signed-off-by: Michael Kubacki <michael.kubacki@microsoft.com> Reviewed-by: Liming Gao <gaoliming@byosoft.com.cn>
Fixes CodeQL alerts for CWE-457: https://cwe.mitre.org/data/definitions/457.html Note that this change affects the actual return value from the following functions. The functions documented that if an integer overflow occurred, MAX_UINTN would be returned. They were implemented to actually return an undefined value from the stack. This change makes the function follow its description. However, this is technically different than what callers may have previously expected. MdePkg/Library/BaseLib/String.c: - StrDecimalToUintn() - StrDecimalToUint64() - StrHexToUintn() - StrHexToUint64() - AsciiStrDecimalToUintn() - AsciiStrDecimalToUint64() - AsciiStrHexToUintn() - AsciiStrHexToUint64() Cc: Erich McMillan <emcmillan@microsoft.com> Cc: Liming Gao <gaoliming@byosoft.com.cn> Cc: Michael D Kinney <michael.d.kinney@intel.com> Cc: Michael Kubacki <mikuback@linux.microsoft.com> Cc: Zhiguang Liu <zhiguang.liu@intel.com> Co-authored-by: Erich McMillan <emcmillan@microsoft.com> Signed-off-by: Michael Kubacki <michael.kubacki@microsoft.com> Reviewed-by: Liming Gao <gaoliming@byosoft.com.cn>
Fixes CodeQL alerts for CWE-457: https://cwe.mitre.org/data/definitions/457.html Cc: Erich McMillan <emcmillan@microsoft.com> Cc: Jiaxin Wu <jiaxin.wu@intel.com> Cc: Maciej Rabeda <maciej.rabeda@linux.intel.com> Cc: Michael D Kinney <michael.d.kinney@intel.com> Cc: Michael Kubacki <mikuback@linux.microsoft.com> Cc: Siyuan Fu <siyuan.fu@intel.com> Co-authored-by: Erich McMillan <emcmillan@microsoft.com> Signed-off-by: Michael Kubacki <michael.kubacki@microsoft.com>
Fixes CodeQL alerts for CWE-457: https://cwe.mitre.org/data/definitions/457.html Cc: Erich McMillan <emcmillan@microsoft.com> Cc: Michael D Kinney <michael.d.kinney@intel.com> Cc: Michael Kubacki <mikuback@linux.microsoft.com> Cc: Ray Ni <ray.ni@intel.com> Co-authored-by: Erich McMillan <emcmillan@microsoft.com> Signed-off-by: Michael Kubacki <michael.kubacki@microsoft.com> Reviewed-by: Michael D Kinney <michael.d.kinney@intel.com> Reviewed-by: Ray Ni <ray.ni@intel.com>
Fixes CodeQL alerts for CWE-457: https://cwe.mitre.org/data/definitions/457.html Cc: Erich McMillan <emcmillan@microsoft.com> Cc: Michael D Kinney <michael.d.kinney@intel.com> Cc: Michael Kubacki <mikuback@linux.microsoft.com> Cc: Ray Ni <ray.ni@intel.com> Cc: Zhichao Gao <zhichao.gao@intel.com> Co-authored-by: Erich McMillan <emcmillan@microsoft.com> Signed-off-by: Michael Kubacki <michael.kubacki@microsoft.com> Reviewed-by: Zhichao Gao <zhichao.gao@intel.com>
Fixes CodeQL alerts for CWE-457: https://cwe.mitre.org/data/definitions/457.html Cc: Eric Dong <eric.dong@intel.com> Cc: Erich McMillan <emcmillan@microsoft.com> Cc: Michael D Kinney <michael.d.kinney@intel.com> Cc: Michael Kubacki <mikuback@linux.microsoft.com> Cc: Rahul Kumar <rahul1.kumar@intel.com> Cc: Ray Ni <ray.ni@intel.com> Co-authored-by: Erich McMillan <emcmillan@microsoft.com> Signed-off-by: Michael Kubacki <michael.kubacki@microsoft.com>
The previous commits fixed issues with these queries across various packages. Now that those are resolved, enable the queries in the edk2 query set so regressions can be found in the future. Enables: 1. cpp/conditionallyuninitializedvariable - CWE: https://cwe.mitre.org/data/definitions/457.html - @name Conditionally uninitialized variable - @description An initialization function is used to initialize a local variable, but the returned status code is not checked. The variable may be left in an uninitialized state, and reading the variable may result in undefined behavior. - @kind problem - @problem.severity warning - @security-severity 7.8 - @id cpp/conditionally-uninitialized-variable - @tags security - external/cwe/cwe-457 2. cpp/pointer-overflow-check - CWE: https://cwe.mitre.org/data/definitions/758.html - @name Pointer overflow check - @description Adding a value to a pointer to check if it overflows relies on undefined behavior and may lead to memory corruption. - @kind problem - @problem.severity error - @security-severity 2.1 - @precision high - @id cpp/pointer-overflow-check - @tags reliability - security - external/cwe/cwe-758 3. cpp/potential-buffer-overflow - CWE: https://cwe.mitre.org/data/definitions/676.html - @name Potential buffer overflow - @description Using a library function that does not check buffer bounds requires the surrounding program to be very carefully written to avoid buffer overflows. - @kind problem - @id cpp/potential-buffer-overflow - @problem.severity warning - @security-severity 10.0 - @tags reliability - security - external/cwe/cwe-676 - @deprecated This query is deprecated, use Potentially overrunning write (`cpp/overrunning-write`) and Potentially overrunning write with float to string conversion (`cpp/overrunning-write-with-float`) instead. Note that cpp/potential-buffer-overflow is deprecated. This query will be updated to the succeeding queries in the next commit. The query is used in this commit to show that we considered and tested the query in history. Cc: Sean Brogan <sean.brogan@microsoft.com> Cc: Michael Kubacki <mikuback@linux.microsoft.com> Cc: Michael D Kinney <michael.d.kinney@intel.com> Signed-off-by: Michael Kubacki <michael.kubacki@microsoft.com> Reviewed-by: Michael D Kinney <michael.d.kinney@intel.com>
As recommended by CodeQL this change replaces cpp/potential-buffer-overflow with cpp/overrunning-write-with-float and cpp/overrunning-write. Enables: 1. cpp/overrunning-write - @name Likely overrunning write - @description Buffer write operations that do not control the length data written may overflow - @kind problem - @problem.severity error - @security-severity 9.3 - @precision high - @id cpp/very-likely-overrunning-write - @tags reliability - security - external/cwe/cwe-120 - external/cwe/cwe-787 - external/cwe/cwe-805 2. cpp/overrunning-write-with-float - @name Potentially overrunning write with float to string conversion - @description Buffer write operations that do not control the length of data written may overflow when floating point inputs take extreme values. - @kind problem - @problem.severity error - @security-severity 9.3 - @precision medium - @id cpp/overrunning-write-with-float - @tags reliability - security - external/cwe/cwe-120 - external/cwe/cwe-787 - external/cwe/cwe-805 3. cpp/very-likely-overrunning-write - @name Likely overrunning write - @description Buffer write operations that do not control the length of data written may overflow - @kind problem - @problem.severity error - @security-severity 9.3 - @precision high - @id cpp/very-likely-overrunning-write - @tags reliability - security - external/cwe/cwe-120 - external/cwe/cwe-787 - external/cwe/cwe-805 - CWEs: - https://cwe.mitre.org/data/definitions/120.html - https://cwe.mitre.org/data/definitions/787.html - https://cwe.mitre.org/data/definitions/805.html Cc: Sean Brogan <sean.brogan@microsoft.com> Cc: Michael Kubacki <mikuback@linux.microsoft.com> Cc: Michael D Kinney <michael.d.kinney@intel.com> Signed-off-by: Michael Kubacki <michael.kubacki@microsoft.com> Reviewed-by: Michael D Kinney <michael.d.kinney@intel.com>
makubacki
force-pushed
the
enable_codeql_condunit_pointer_buffer_overflow_v4
branch
from
March 10, 2023 16:35
c71d38a
to
d803f03
Compare
Superseded by #4175. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
v4 of patch series.