Skip to content
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

Default to ASN TEMPLATE library #7199

Merged
merged 2 commits into from
Feb 6, 2024
Merged

Conversation

lealem47
Copy link
Contributor

@lealem47 lealem47 commented Feb 1, 2024

Description

user_settings.h users will have to define WOLFSSL_ASN_ORIGINAL to use the old ASN library.

Fixes zd#

Testing

How did you test?

Checklist

  • added tests
  • updated/added doxygen
  • updated appropriate READMEs
  • Updated manual and documentation

@lealem47 lealem47 self-assigned this Feb 1, 2024
SparkiDev
SparkiDev previously approved these changes Feb 1, 2024
@lealem47
Copy link
Contributor Author

lealem47 commented Feb 1, 2024

Jenkins Retest this please

dgarske
dgarske previously approved these changes Feb 2, 2024
Copy link
Contributor

@douzzer douzzer left a comment

Choose a reason for hiding this comment

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

This all looks good.

The change in settings.h had an unexpected consequence for cppcheck static analysis. cppcheck is clever enough to see WOLFSSL_ASN_TEMPLATE forced on now, but it's not clever enough to follow the arcane pointer jujitsu around GetASNItems(), so now we get this:

[cppcheck-all] [1 of 1] [5b1ead35f3-dirty]
    autogen.sh 5b1ead35f3-dirty...   real 0m16.322s  user 0m14.129s  sys 0m1.336s
    configure...   real 0m18.429s  user 0m6.212s  sys 0m14.187s
    cppcheck...d486b89c61 (<sean@wolfssl.com> 2020-12-17 13:41:56 +1000 3032)         *version = num;
wolfcrypt/src/asn.c:3032:20: error: Uninitialized variable: num [uninitvar]
        *version = num;
                   ^
6418e3cbfe (<david@wolfssl.com> 2023-04-04 13:46:30 -0700 3099)         *number = (int)num;
wolfcrypt/src/asn.c:3099:24: error: Uninitialized variable: num [uninitvar]
        *number = (int)num;
                       ^
d486b89c61 (<sean@wolfssl.com> 2020-12-17 13:41:56 +1000 6801)         if (version > PKCS8v1) {
wolfcrypt/src/asn.c:6801:13: error: Uninitialized variable: version [uninitvar]
        if (version > PKCS8v1) {
            ^
7155c5748e (<douzzer@wolfssl.com> 2023-04-10 19:05:09 -0500 8771)             password, passwordSz, salt, (int)saltSz, (int)iterations, id, key,
wolfcrypt/src/asn.c:8771:59: error: Uninitialized variable: iterations [uninitvar]
            password, passwordSz, salt, (int)saltSz, (int)iterations, id, key,
                                                          ^
cebb4da307 (<douzzer@wolfssl.com> 2023-07-25 11:31:01 -0500 33119)         if (version != 1) {
wolfcrypt/src/asn.c:33119:13: error: Uninitialized variable: version [uninitvar]
        if (version != 1) {
            ^
d486b89c61 (<sean@wolfssl.com> 2020-12-17 13:41:56 +1000 36135)         resp->responseStatus = status;
wolfcrypt/src/asn.c:36135:32: error: Uninitialized variable: status [uninitvar]
        resp->responseStatus = status;
                               ^
   real 0m54.087s  user 14m13.258s  sys 1m1.623s
    cppcheck-all fail_analysis
    failed config: '--enable-all' '--enable-testcert' '--enable-srtp'

These are definitely false positives -- I tested its basic understanding of the code pattern with this:

diff --git a/wolfcrypt/src/asn.c b/wolfcrypt/src/asn.c
index 40ab40300..1003cc25c 100644
--- a/wolfcrypt/src/asn.c
+++ b/wolfcrypt/src/asn.c
@@ -3023,6 +3023,7 @@ int GetMyVersion(const byte* input, word32* inOutIdx,
     /* Clear dynamic data and set the version number variable. */
     XMEMSET(dataASN, 0, sizeof(dataASN));
     GetASN_Int8Bit(&dataASN[INTASN_IDX_INT], &num);
+dataASN[INTASN_IDX_INT].data.u8 = 0;
     /* Decode the version (INTEGER). */
     ret = GetASN_Items(intASN, dataASN, intASN_Length, 0, input, inOutIdx,
                        maxIdx);

and it still reported num was uninitialized.

So this is the easy-peasy fix:

diff --git a/wolfcrypt/src/asn.c b/wolfcrypt/src/asn.c
index 40ab40300..e5d5df172 100644
--- a/wolfcrypt/src/asn.c
+++ b/wolfcrypt/src/asn.c
@@ -3018,7 +3018,7 @@ int GetMyVersion(const byte* input, word32* inOutIdx,
 #else
     ASNGetData dataASN[intASN_Length];
     int ret;
-    byte num;
+    byte num = 0;
 
     /* Clear dynamic data and set the version number variable. */
     XMEMSET(dataASN, 0, sizeof(dataASN));
@@ -3085,7 +3085,7 @@ int GetShortInt(const byte* input, word32* inOutIdx, int* number, word32 maxIdx)
 #else
     ASNGetData dataASN[intASN_Length];
     int ret;
-    word32 num;
+    word32 num = 0;
 
     /* Clear dynamic data and set the 32-bit number variable. */
     XMEMSET(dataASN, 0, sizeof(dataASN));
@@ -6767,7 +6767,7 @@ int ToTraditionalInline_ex(const byte* input, word32* inOutIdx, word32 sz,
     DECL_ASNGETDATA(dataASN, pkcs8KeyASN_Length);
     int ret = 0;
     word32 oid = 9;
-    byte version;
+    byte version = 0;
     word32 idx;
 
     /* Check validity of parameters. */
@@ -8685,7 +8685,7 @@ exit_dc:
     int    version;
     word32 idx = 0;
     word32 pIdx = 0;
-    word32 iterations;
+    word32 iterations = 0;
     word32 keySz = 0;
     word32 saltSz = 0;
     word32 shaOid = 0;
@@ -33081,7 +33081,7 @@ int wc_EccPrivateKeyDecode(const byte* input, word32* inOutIdx, ecc_key* key,
     return ret;
 #else
     DECL_ASNGETDATA(dataASN, eccKeyASN_Length);
-    byte version;
+    byte version = 0;
     int ret = 0;
     int curve_id = ECC_CURVE_DEF;
 #if defined(HAVE_PKCS8) || defined(HAVE_PKCS12) || defined(SM2)
@@ -36113,7 +36113,7 @@ int OcspResponseDecode(OcspResponse* resp, void* cm, void* heap, int noVerify)
     int ret = 0;
     word32 idx = 0, size = resp->maxIdx;
     byte* source = resp->source;
-    byte status;
+    byte status = 0;
     byte* basic;
     word32 basicSz;

I'll leave it to you to apply that, with or without your own personal touches.

I'm running cppcheck-2.12.1 here. The exact cppcheck invocation with all its command line args is 13K long (really!) so if you want to reproduce you should do it on Linux with

../testing/git-hooks/wolfssl-multi-test.sh --enable-bwrap --enable-git-blame -j 8 --keep-going --max-check-try-count=3 --no-result-cache --verbose-analyzers --test-uncommitted --no-git-refresh cppcheck-all

@douzzer douzzer assigned lealem47 and unassigned wolfSSL-Bot Feb 2, 2024
@douzzer
Copy link
Contributor

douzzer commented Feb 5, 2024

retest this please

@lealem47 lealem47 requested a review from douzzer February 6, 2024 00:16
@lealem47 lealem47 assigned wolfSSL-Bot and douzzer and unassigned lealem47 Feb 6, 2024
@douzzer douzzer merged commit 4d842f0 into wolfSSL:master Feb 6, 2024
110 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants