Modify AES-CTR to not reinit after being keyed

[ColtonWilley]

No description provided.

[ColtonWilley]

Retest this please

[SparkiDev]

Is there an explicit test for this changes?

[ColtonWilley]

Is there an explicit test for this changes?

This re-init flow already exists in test_aes_ctr_leftover_data_regression(). It used to work before commit wolfSSL/wolfssl@6b8280f, but that commit changes wc_AesInit() to set rounds to zero, thus causing a new but valid error from wc_AesEncrypt() because rounds is zero.

[kaleb-himes]

Is there an explicit test for this changes?

This re-init flow already exists in test_aes_ctr_leftover_data_regression(). It used to work before commit wolfSSL/wolfssl@6b8280f, but that commit changes wc_AesInit() to set rounds to zero, thus causing a new but valid error from wc_AesEncrypt() because rounds is zero.

I may have misunderstood but I think @SparkiDev was asking if an explicit test will be added to cover the case.

The issue identified came from the call sequence: init-with-key then init-iv where the init-iv wiped out the rounds set by init-with-key

Possibly adding a test in tests/api.c that checks the rounds after a double init call? (Pseudo code below)

int test_aes_ctr_multi_init()
{
     EXPECT_DECLS;
     int rounds_expected = #rounds expected to be there as a result of the first call init'ing with key;
     /* Test valid sequence */
     ExpectIntEQ(we_aes_ctr_init(with key set), WOLFSSL_SUCCESS);
     ExpectIntEQ(we_aes_ctr_init(with all the NULL params and only the IV passed in), WOLFSSL_SUCCESS);
     ExpectIntEQ(rounds in the ctx, rounds_expected);
     return EXPECT_RESULT();
}

[ColtonWilley]

Possibly adding a test in tests/api.c that checks the rounds after a double init call?

If such tests were to be added, that should be a direct test of the wolfcrypt APIs, I dont think wolfengine tests are a good place for verifying AES context internals. We could add another test with just a double init, but it would ultimately be a copy of the existing test code. If the consensus is that we absolutely want new test code I can add some, but I think it would be a copy of existing code that doesnt actually add anything new in terms of testing.

[kaleb-himes]

Fair enough.

[bandi13]

The failure in gcc, clang, scan-build, fsanitize and disable test(s) can be ignored. It was a test that accidentally ran on this PR. It has been disabled.

[douzzer]

LGTM. as discussed, more fixes will be needed for more leaks around wc_AesInit(), and possibly for more over-inits.