Skip to content

Unwind Issue: How to make a loop inductive? #8353

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

Closed
QinyuanWu opened this issue Jun 20, 2024 · 49 comments
Closed

Unwind Issue: How to make a loop inductive? #8353

QinyuanWu opened this issue Jun 20, 2024 · 49 comments
Assignees

Comments

@QinyuanWu
Copy link

QinyuanWu commented Jun 20, 2024

I'm verifying the SymCrypt library and I'm running into unwinding failure for the SymCryptWipeAsm function, which is a simple loop that sets the buffer to 0, but because the loop iteration depends on cbData, I believe CBMC want to unwind it to the max value of cbData(under the proof constrain of 1024). I'm trying to specify invariants to make this loop inductive so that CBMC only unwinds once. I've also tried using __CPROVER_ensures with __CPROVER_forall as specified in this documentation, but I got a parsing error from goto-cc.
Harness(SymCryptWipeAsm is implemented here to overwrite the assembly version):

#include <stdlib.h>
#include "symcrypt.h"

SYMCRYPT_ENVIRONMENT_LINUX_USERMODE

void harness(void)
{
    SIZE_T cbData; // unconstrained value
    PBYTE pbData;
    BYTE abResult[SYMCRYPT_SHA256_RESULT_SIZE];

    __CPROVER_assume(cbData <= 1024);
    pbData = malloc( cbData );

    __CPROVER_assume(pbData != NULL);

    SymCryptSha256( pbData, cbData, abResult );

    free(pbData);
}

VOID
SYMCRYPT_CALL
SymCryptWipeAsm( _Out_writes_bytes_( cbData ) PVOID pbData, SIZE_T cbData )
{
    volatile BYTE * p = (volatile BYTE *) pbData;
    SIZE_T i;

    __CPROVER_assume( pbData != NULL );
    __CPROVER_assume( __CPROVER_w_ok( pbData, cbData ) );

    for( i=0; i<cbData; i++ )
    __CPROVER_loop_invariant( 0 <= i && i < cbData )
    __CPROVER_decreases( cbData - i )
    {
        p[i] = 0;
    }

/* What I've also tried but got parsing error:
    for( i=0; i<cbData; i++ )
    __CPROVER_ensures(__CPROVER_forall {
    SIZE_T i;
    (0 <= i && i < SIZE_MAX) ==>
      p[i]==0
  })
    {
        p[i] = 0;
    }
*/

}

Related source files:
PROJECT_SOURCES += $(SRCDIR)/lib/sha256.c
PROJECT_SOURCES += $(SRCDIR)/lib/env_linuxUserMode.c
PROJECT_SOURCES += $(SRCDIR)/lib/libmain.c
PROJECT_SOURCES += $(SRCDIR)/lib/sha256Par.c
image

CBMC version: 5.95.1
Operating system: WSL
What behaviour did you expect: The loop become inductive and CBMC unwind once
What happened instead: unwinding failure for unwind --32
Please let me know if you need more information and I appreciate the help!

@QinyuanWu
Copy link
Author

I'm also wondering about general approach to make loops inductive(what's possible and what's not with CBMC) and if there's any documentation on that.

@qinheping
Copy link
Collaborator

qinheping commented Jun 21, 2024

Yes, CBMC support loop contracts to prove loops inductively: https://github.com/diffblue/cbmc/blob/develop/src/goto-instrument/contracts/doc/user/contracts-loop-invariants.md.
You can also find more examples in cbmc/regression/contracts-dfcc/loop_* benchmarks.

In your example, the clause __CPROVER_ensures is for function contracts. You may want to use loop contract clauses __CPROVER_loop_invariant instead.

@rod-chapman
Copy link
Collaborator

You might take a look at my "cbmc-examples" repository. This contains a selection of simple functions and their verification. All the examples use contracts (including loop invariants) to show how unbounded verification can be done with CBMC.

The "arrays" subdirectory there includes function that do array initialization, assignment, and some cryptographic things like constant-time equality and conditional copy.

See https://github.com/rod-chapman/cbmc-examples/

  • Rod (AWS Cryptography)

@QinyuanWu
Copy link
Author

QinyuanWu commented Jun 21, 2024

Yes, CBMC support loop contracts to prove loops inductively: https://github.com/diffblue/cbmc/blob/develop/src/goto-instrument/contracts/doc/user/contracts-loop-invariants.md. You can also find more examples in cbmc/regression/contracts-dfcc/loop_* benchmarks.

In your example, the clause __CPROVER_ensures is for function contracts. You may want to use loop contract clauses __CPROVER_loop_invariant instead.

Thank you for the reply! In my harness I used __CPROVER_loop_invariant( 0 <= i && i < cbData ) and __CPROVER_decreases( cbData - i ) but CBMC still tries to unwind the loop more than once. Am I still missing something for the invariant?

@qinheping
Copy link
Collaborator

Yes, CBMC support loop contracts to prove loops inductively: https://github.com/diffblue/cbmc/blob/develop/src/goto-instrument/contracts/doc/user/contracts-loop-invariants.md. You can also find more examples in cbmc/regression/contracts-dfcc/loop_* benchmarks.
In your example, the clause __CPROVER_ensures is for function contracts. You may want to use loop contract clauses __CPROVER_loop_invariant instead.

Thank you for the reply! In my harness I used __CPROVER_loop_invariant( 0 <= i && i < cbData ) and __CPROVER_decreases( cbData - i ) but CBMC still tries to unwind the loop more than once. Am I still missing something for the invariant?

After running goto-cc to generate the corresponding a.goto on your c code, you can then run

goto-instrument --dfcc $(TARGET)_harness --apply-loop-contracts a.out b.out

to instrument loop contracts in a.out and generate b.out, where $(TARGET)_harness is the name of your harness and should be harness in your example. Then running

cbmc b.out

will prove the harness with loop contracts.

If you don't want to use dynamic frame condition checking (https://diffblue.github.io/cbmc/contracts-dev-spec-dfcc.html), instrumenting loop contracts with

goto-instrument  --apply-loop-contracts a.out b.out

instead.

@QinyuanWu
Copy link
Author

@qinheping I'm using cbmc-starter-kit to run the proof and I added the --apply-loop-contracts flag to the makefile. I have another proof for SymCryptMd2, and I annotated loop invariants for this loop in SymCryptMd2AppendBlocks in the hope that CBMC does not need to unwind 18*48 times:

        t = 0;
        for( j=0; j<18; j++ )
        __CPROVER_loop_invariant(0 <= j < 18)
        __CPROVER_decreases(18 - j)
        {
            for( k=0; k<48; k++ )
            __CPROVER_loop_invariant(0 <= k < 48)
            __CPROVER_decreases(48 - k)
            {
                t = pChain->X[k] ^ SymCryptMd2STable[t];
                pChain->X[k] = (BYTE) t;
            }
            t = (t + j)& 0xff;
        }

but when I run make goto-instrument reported the following error:

[9/16] SymCryptMd2: checking function contracts                                                                                                                                                                          FAILED: /home/jiggly/test-cbmc/SymCrypt-CBMC/CBMC/proofs/SymCryptMd2/gotos/SymCryptMd2_harness0600.goto /tmp/litani/runs/1ff454d7-40fb-4fce-b0e7-ba48e7025770/status/63e5961c-a1ae-4ea1-bd00-e4ad34f4e6be.json 

/usr/libexec/litani/litani exec --command 'goto-instrument      --apply-loop-contracts /home/jiggly/test-cbmc/SymCrypt-CBMC/CBMC/proofs/SymCryptMd2/gotos/SymCryptMd2_harness0500.goto /home/jiggly/test-cbmc/SymCrypt-CBMC/CBMC/proofs/SymCryptMd2/gotos/SymCryptMd2_harness0600.goto' --pipeline-name SymCryptMd2 --ci-stage build --job-id 63e5961c-a1ae-4ea1-bd00-e4ad34f4e6be --stdout-file /home/jiggly/test-cbmc/SymCrypt-CBMC/CBMC/proofs/SymCryptMd2/logs/check_function_contracts-log.txt --description 'SymCryptMd2: checking function contracts' --status-file /tmp/litani/runs/1ff454d7-40fb-4fce-b0e7-ba48e7025770/status/63e5961c-a1ae-4ea1-bd00-e4ad34f4e6be.json --profile-memory-interval 10 --inputs /home/jiggly/test-cbmc/SymCrypt-CBMC/CBMC/proofs/SymCryptMd2/gotos/SymCryptMd2_harness0500.goto --outputs /home/jiggly/test-cbmc/SymCrypt-CBMC/CBMC/proofs/SymCryptMd2/gotos/SymCryptMd2_harness0600.goto

--- begin invariant violation report ---

Invariant check failed

File: ../src/goto-instrument/contracts/instrument_spec_assigns.h:286 function: update

Condition: source_location.is_not_nil()

Reason: Precondition

Backtrace:

goto-instrument(+0x902510) [0x5571b6055510]

goto-instrument(+0x9031e9) [0x5571b60561e9]

goto-instrument(+0x185244) [0x5571b58d8244]

goto-instrument(+0x2fedcd) [0x5571b5a51dcd]

goto-instrument(+0x2fc09b) [0x5571b5a4f09b]

goto-instrument(+0x2fd11e) [0x5571b5a5011e]

goto-instrument(+0x277f82) [0x5571b59caf82]

goto-instrument(+0x27d228) [0x5571b59d0228]

goto-instrument(+0x27d8b0) [0x5571b59d08b0]

goto-instrument(+0x18abcd) [0x5571b58ddbcd]

goto-instrument(+0x18ec17) [0x5571b58e1c17]

goto-instrument(+0x182a2f) [0x5571b58d5a2f]

goto-instrument(+0x172a69) [0x5571b58c5a69]

/lib/x86_64-linux-gnu/libc.so.6(+0x29d90) [0x7f6ee96e0d90]

/lib/x86_64-linux-gnu/libc.so.6(__libc_start_main+0x80) [0x7f6ee96e0e40]

goto-instrument(+0x184295) [0x5571b58d7295]





--- end invariant violation report ---

Aborted

ninja: build stopped: cannot make progress due to previous errors.

@qinheping
Copy link
Collaborator

qinheping commented Jun 25, 2024

I tried to reproduced the issue with a smaller example and observed the same error.

typedef unsigned SIZE_T;
typedef uint8_t BYTE;
typedef BYTE *PBYTE;
typedef const BYTE *PCBYTE;
#define NULL ((void *)0)
#define SYMCRYPT_MD2_RESULT_SIZE (16)
#define SYMCRYPT_MD2_INPUT_BLOCK_SIZE (16)

const BYTE SymCryptMd2STable[256] = {
  41,  46,  67,  201, 162, 216, 124, 1,   61,  54,  84,  161, 236, 240, 6,
  19,  98,  167, 5,   243, 192, 199, 115, 140, 152, 147, 43,  217, 188, 76,
  130, 202, 30,  155, 87,  60,  253, 212, 224, 22,  103, 66,  111, 24,  138,
  23,  229, 18,  190, 78,  196, 214, 218, 158, 222, 73,  160, 251, 245, 142,
  187, 47,  238, 122, 169, 104, 121, 145, 21,  178, 7,   63,  148, 194, 16,
  137, 11,  34,  95,  33,  128, 127, 93,  154, 90,  144, 50,  39,  53,  62,
  204, 231, 191, 247, 151, 3,   255, 25,  48,  179, 72,  165, 181, 209, 215,
  94,  146, 42,  172, 86,  170, 198, 79,  184, 56,  210, 150, 164, 125, 182,
  118, 252, 107, 226, 156, 116, 4,   241, 69,  157, 112, 89,  100, 113, 135,
  32,  134, 91,  207, 101, 230, 45,  168, 2,   27,  96,  37,  173, 174, 176,
  185, 246, 28,  70,  97,  105, 52,  64,  126, 15,  85,  71,  163, 35,  221,
  81,  175, 58,  195, 92,  249, 206, 186, 197, 234, 38,  44,  83,  13,  110,
  133, 40,  132, 9,   211, 223, 205, 244, 65,  129, 77,  82,  106, 220, 55,
  200, 108, 193, 171, 250, 36,  225, 123, 8,   12,  189, 177, 74,  120, 136,
  149, 139, 227, 99,  232, 109, 233, 203, 213, 254, 59,  0,   29,  57,  242,
  239, 183, 14,  102, 88,  208, 228, 166, 119, 114, 248, 235, 117, 75,  10,
  49,  68,  80,  180, 143, 237, 31,  26,  219, 153, 141, 51,  159, 17,  131,
  20};
typedef struct
{
  BYTE C[16]; // State for internal checksum computation
  BYTE X[48]; // State for actual hash chaining
} STATE;

void SymCryptMd2AppendBlocks(
  STATE *pChain,
  PCBYTE pbData,
  SIZE_T cbData,
  SIZE_T *pcbRemaining)
{
  //
  // For variable names see RFC 1319.
  //
  unsigned int t;
  int j, k;

  while(cbData >= SYMCRYPT_MD2_INPUT_BLOCK_SIZE)
  {
    BYTE L;
    L = pChain->C[15];

    for(j = 0; j < 16; j++)
    {
      pChain->C[j] = L =
        pChain->C[j] ^ SymCryptMd2STable[L ^ pChain->X[16 + j]];
    }

    t = 0;
    for(j = 0; j < 18; j++)
      __CPROVER_loop_invariant(0 <= j < 18) __CPROVER_decreases(18 - j)
      {
        for(k = 0; k < 48; k++)
          __CPROVER_loop_invariant(0 <= k < 48) __CPROVER_decreases(48 - k)
          {
            t = pChain->X[k] ^ SymCryptMd2STable[t];
            pChain->X[k] = (BYTE)t;
          }
        t = (t + j) & 0xff;
      }

    pbData += SYMCRYPT_MD2_INPUT_BLOCK_SIZE;
    cbData -= SYMCRYPT_MD2_INPUT_BLOCK_SIZE;
  }

  *pcbRemaining = cbData;
}
void main()
{
  SIZE_T cbData; // unconstrained value
  SIZE_T *pcbRemaining;
  PBYTE pbData;
  BYTE abResult[SYMCRYPT_MD2_RESULT_SIZE];
  __CPROVER_assume(cbData <= 8);
  pbData = malloc(cbData);

  BYTE C[16]; // State for internal checksum computation
  BYTE X[48]; // State for actual hash chaining
  STATE state = {C, X};

  __CPROVER_assume(pbData != NULL);

  SymCryptMd2AppendBlocks(&state, pbData, cbData, pcbRemaining);

  free(pbData);
}

I believe the issue is caused by writing to field X of struct pChain in the loop.
While I am fixing the issue, could you try to use dfcc loop contracts instead? I didn't get the error with dfcc enabled. You can use dfcc by adding the dfcc flag before --apply-loop-contracts. It will be

goto-instrument --dfcc HARNESS_NAME  --apply-loop-contracts goto.out

Where HARNESS_NAME is the name of the harness function, and should be harness in SymCryptMd2.

You may also want to write loop contracts for the outer while-loop to avoid long verification time.

@QinyuanWu
Copy link
Author

QinyuanWu commented Jun 25, 2024

Thank you @qinheping. So I added the --dfcc flag but the pipeline failed at the checking property step, and I saw the warning that the loops are missing assigns clauses. I followed the __CPROVER_assigns documentation and added the clauses as below but received parsing error.
Assign clause warning:
image
Failed property check(I also tried reducing the checks to only --bound-check but still failed):
image
I added assigns clause to the following three loops:
In the harness:

VOID
SYMCRYPT_CALL
SymCryptWipeAsm( _Out_writes_bytes_( cbData ) PVOID pbData, SIZE_T cbData )
{
    volatile BYTE * p = (volatile BYTE *) pbData;
    SIZE_T i;

    __CPROVER_assume( pbData != NULL );
    __CPROVER_assume( __CPROVER_w_ok( pbData, cbData ) );
    

    for( i=0; i<cbData; i++ )
    __CPROVER_loop_invariant( 0 <= i && i < cbData )
    __CPROVER_decreases( cbData - i )
    __CPROVER_assigns( __CPROVER_typed_target(p[i]) )
    {
        p[i] = 0;
    }

}

In md2.c SymCryptMd2AppendBlocks:

        t = 0;
        for( j=0; j<18; j++ )
        __CPROVER_assigns(__CPROVER_typed_target(t))
        __CPROVER_loop_invariant(0 <= j < 18)
        __CPROVER_decreases(18 - j)
        {
            for( k=0; k<48; k++ )
            __CPROVER_assigns(__CPROVER_typed_target(t))
            __CPROVER_assigns(__CPROVER_typed_target(pChain->X[k]))
            __CPROVER_loop_invariant(0 <= k < 48)
            __CPROVER_decreases(48 - k)
            {
                t = pChain->X[k] ^ SymCryptMd2STable[t];
                pChain->X[k] = (BYTE) t;
            }
            t = (t + j)& 0xff;
        }

Assigns clause parsing error:
image
I'm running CBMC 6.0.1 right now.

@qinheping
Copy link
Collaborator

Loop contracts have to be specified in order. __CPROVER_assigns must be specified before __CPROVER_loop_invariant. That is, it should be

    for( i=0; i<cbData; i++ )
    __CPROVER_assigns( __CPROVER_typed_target(p[i]) )
    __CPROVER_loop_invariant( 0 <= i && i < cbData )
    __CPROVER_decreases( cbData - i )
    {
        p[i] = 0;
    }

@QinyuanWu
Copy link
Author

I've switch the order but the error persists. You can see from the parsing error it's referring to the line pChaint->X[k] and the assign clause precedes the other statements.

@qinheping qinheping self-assigned this Jun 25, 2024
@qinheping
Copy link
Collaborator

Failed property check(I also tried reducing the checks to only --bound-check but still failed):

Could you provide more detail about the failure by running the command after /usr/libexec/litani/litani exec --command?

Assigns clause parsing error:

Sorry I didn't notice that you write two assigns clauses for one loop. All assign targets must be specified in one assign clause, for example

for( k=0; k<48; k++ )
            __CPROVER_assigns(__CPROVER_typed_target(t), __CPROVER_typed_target(pChain->X[k]))
            __CPROVER_loop_invariant(0 <= k < 48)
            __CPROVER_decreases(48 - k)
            {
                t = pChain->X[k] ^ SymCryptMd2STable[t];
                pChain->X[k] = (BYTE) t;
            }
            t = (t + j)& 0xff;

@QinyuanWu
Copy link
Author

With the assign clauses now and --dfcc and --apply-loop-contract there is no direct failure but the report is giving me the following warning and errors:
image
Partial trace for first error:
image
Partial trace for second error:
image
SymCryptWipeAsm:
image
With the error CBMC stops at the SymCryptWipeAsm function and does not look further which causes poor coverage.
This is proof for SymCryptMd2 and the full harness is here
I've been getting stuck on this simple function for two weeks now and it seems like it shouldn't be too complicated to make it inductive... My ultimate goal is for CBMC to not give unwinding failure since cbData is of size_t and can be very large. Appreciate any guidance^_^

@rod-chapman
Copy link
Collaborator

You should look at the "init_st" function in my cbmc-examples repository, which does almost exactly the same time.
The specification with pre- and post-condition is here:
https://github.com/rod-chapman/cbmc-examples/blob/b8d5caf4fb39148084511200e1676615e91aefea/arrays/ar.h#L77
the body is here:
https://github.com/rod-chapman/cbmc-examples/blob/b8d5caf4fb39148084511200e1676615e91aefea/arrays/ar.c#L169

Using CBMC 6.0.0, if I run:

make TARGET=init_st

the output concludes:

** 0 of 1246 failed (1 iterations)
VERIFICATION SUCCESSFUL

@rod-chapman
Copy link
Collaborator

I assed a "zero_slice()" function, which is closer to your example.
Specification: https://github.com/rod-chapman/cbmc-examples/blob/823a7094e9dfa3426b1892aa364bdc5febbdf28b/arrays/ar.h#L82
Body: https://github.com/rod-chapman/cbmc-examples/blob/823a7094e9dfa3426b1892aa364bdc5febbdf28b/arrays/ar.c#L181

Just do
"make TARGET=zero_slice"
to see the proof.

@QinyuanWu
Copy link
Author

Thank you @rod-chapman! Using __CPROVER_object_upto() resolved the assignable error, but the first error(Check invariant after step for loop) still persists:
image
My updated harness:

VOID
SYMCRYPT_CALL
SymCryptWipeAsm( _Out_writes_bytes_( cbData ) PVOID pbData, SIZE_T cbData )
{
    volatile BYTE * p = (volatile BYTE *) pbData;
    SIZE_T i;

    for( i=0; i<cbData; i++ )
    __CPROVER_assigns( __CPROVER_typed_target(i), __CPROVER_object_upto(p, cbData) )
    __CPROVER_loop_invariant( i < cbData ) // also tried __CPROVER_loop_invariant( 0 <= i && i < cbData )
    __CPROVER_loop_invariant(__CPROVER_forall { size_t j; (0 <= j && j < i) ==> p[j] == 0 } )
    __CPROVER_decreases( cbData - i ) 
    {
        p[i] = 0;
    }

}

I'm not sure what the error itself means. Is it saying that we should only check invariant after the loop has started or we should not?

@qinheping
Copy link
Collaborator

I'm not sure what the error itself means. Is it saying that we should only check invariant after the loop has started or we should not?

This error means the loop contracts are not inductive. Take your invariant __CPROVER_loop_invariant( i < cbData ) as an example. When one iteration starting with i equals to cbData-1 which satisfied the invariant, i will become cbData at the end of the iteration and break the invariant. I think the invariant for the loop should be i <= cbData.

To get a better idea of why some properties fail, you can run CBMC on the produced goto program with the flag --trace to see the playback of the failure.

@rod-chapman
Copy link
Collaborator

The loop invariant needs to be "i <= cbData", since the invariant is verified just before the loop exit condition gets evaluated. It's not exactly intuitive, but see my examples that I mentioned earlier.

@QinyuanWu
Copy link
Author

QinyuanWu commented Jun 26, 2024

@qinheping I was looking at the proofs for aws c common and aws encryption sdk and it looks like they didn't turn on the --apply-loop-contract, --dfcc or function contract flag, and there was not use of __CPROVER_loop_invariant() clauses. In addition, the unwind flag in Makefile.common are both set to 1. I would imagine unwind bound of 1 would be too small for a lot of the loops and without applying loop contracts how were they able to not get any unwind failure errors?

@QinyuanWu
Copy link
Author

QinyuanWu commented Jun 26, 2024

Also I'm getting a lot of warning:
image
image
I'm not sure what caused these warnings and is there anyway to correct or supress these warnings? Thanks!

@qinheping
Copy link
Collaborator

@qinheping I was looking at the proofs for aws c common and aws encryption sdk and it looks like they didn't turn on the --apply-loop-contract, --dfcc or function contract flag, and there was not use of __CPROVER_loop_invariant() clauses.

You can find the unbounded proof of aws c common here. However, the proofs only use old loop contracts but not dfcc.

In addition, the unwind flag in Makefile.common are both set to 1. I would imagine unwind bound of 1 would be too small for a lot of the loops and without applying loop contracts how were they able to not get any unwind failure errors?

The actual unwinding bounds are specified for each proof separately in the variable UNWINDSET in makefile. https://github.com/search?q=repo%3Aaws%2Faws-encryption-sdk-c+unwindset&type=code.

@qinheping
Copy link
Collaborator

Also I'm getting a lot of warning: image image I'm not sure what caused these warnings and is there anyway to correct or supress these warnings? Thanks!

The third warning ignoring forall. The SAT backend eagerly grounds quantifiers when domains are small, but ignores them when domains are too large, which is seems to be the case here. Is there any quantifier __CPROVER_forall in your code? If yes, you should try using the SMT back-end, z3 for example, by adding the flag --smt2 to the CBMC command.

For the first two warnings, there are duplicate functions. @tautschnig , what is the right way to supress the duplicate symbol warning?

@rod-chapman
Copy link
Collaborator

  1. You should always use --smt2 with quantifiers.
  2. In my cbmc-examples repo, all the functions use contracts, loop invariants and unbounded verification. I hope to expand these to cover more cases as I go along.

I could point you at an entire crypto library that has sound, unbounded verification of type safety, but it ain't C.. :-)

@QinyuanWu
Copy link
Author

I could point you at an entire crypto library that has sound, unbounded verification of type safety, but it ain't C.. :-)

😮 @rod-chapman Could you still give me the reference? What language is it in and what's the verification tool?

@rod-chapman
Copy link
Collaborator

Here's MLKEM (aka "Kyber"): https://github.com/awslabs/LibMLKEM/tree/main/spark_ada
And here's the whole of NaCl: https://github.com/rod-chapman/SPARKNaCl
Language: Ada2012 (SPARK subset)
Toolset: SPARK Pro

@QinyuanWu
Copy link
Author

The execution failed after switching to --smt2 😿
Failed command: cbmc --flush --unwind 1 --unwinding-assertions --smt2 --bounds-check --trace --xml-ui /home/jiggly/SymCrypt-CBMC/CBMC/proofs/SymCryptMd2/gotos/SymCryptMd2_harness.goto
image
Upon checking the log the solver had an error in this step:
image

@qinheping
Copy link
Collaborator

The execution failed after switching to --smt2 😿 Failed command: cbmc --flush --unwind 1 --unwinding-assertions --smt2 --bounds-check --trace --xml-ui /home/jiggly/SymCrypt-CBMC/CBMC/proofs/SymCryptMd2/gotos/SymCryptMd2_harness.goto image Upon checking the log the solver had an error in this step: image

The image tells that there is some error when running SMT2 solver, but it doesn't show the actual error.I suppose it is the same issue as #8329. Could you find and share the accrual error message. Searching for SMT2 solver returned error message: might help.

@QinyuanWu
Copy link
Author

@qinheping I believe it's the same issue with #8329 since I'm also using the for_all statement in a similar way:

VOID
SYMCRYPT_CALL
SymCryptWipeAsm( _Out_writes_bytes_( cbData ) PVOID pbData, SIZE_T cbData )
{
    volatile BYTE * p = (volatile BYTE *) pbData;
    SIZE_T i;

    for( i=0; i<cbData; i++ )
    __CPROVER_assigns( __CPROVER_typed_target(i), __CPROVER_object_upto(p, cbData) )
    __CPROVER_loop_invariant( i < cbData ) // also tried __CPROVER_loop_invariant( 0 <= i && i < cbData )
    __CPROVER_loop_invariant(__CPROVER_forall { size_t j; (0 <= j && j < i) ==> p[j] == 0 } ) // forall x. e1 => e2
    __CPROVER_decreases( cbData - i ) 
    {
        p[i] = 0;
    }

}

What's the best approach right now? If SAT and SMT both are not able to evaluate the for_all statment correctly is there another way to make this loop inductive? I just tried switching back to SAT and re-run the proof for SymCryptMd2 and it gave the following errors:
image
The induction error persists
image
And I borrowed the stub for memcpy from aws and I'm not sure what triggered these errors and what need to be done in general to prevent these type of dereference failures:
image
Also the __CPROVER_contracts library also has some complaints(is this the result of using the inadequate solver?):
image

@QinyuanWu
Copy link
Author

QinyuanWu commented Jun 28, 2024

@qinheping I followed up with the SymCrypt dev team on the destination writable errors we found in sat_not_stub_result:
[SymCryptHashAppendInternal.precondition_instance.6] line 76 memcpy destination region writeable: FAILURE
Line of code:
memcpy( &pState->buffer[bytesInBuffer], pbData, cbData );
This one may be because the pState struct referred here is the PSYMCRYPT_COMMON_HASH_STATE which is the generic hash state struct that has a buffer element of size 1(#define SYMCRYPT_ANYSIZE_ARRAY 1), but when the actual hash function is invoked depending on the algorithm different state struct is passed in that has larger allocated buffer.
image
For example SYMCRYPT_MD2_STATE allocate 16 bytes:
image
These structs are defined in symcrypt_internal.h. This is essentially an implicit morphism and CBMC may not be interpreting that.
For the second error:
[SymCryptMd2Result.precondition_instance.1] line 125 memset destination region writeable: FAILURE
Line of code:
memset( &state->buffer[state->bytesInBuffer], (BYTE)paddingBytes, paddingBytes );
I think the issue here is that bytesInBuffer is an unsigned integer which can potentially have a value greater than 16 and cause an underflow. Is it best to add __CPROVER_assert(state->bytesInBuffer <= 16) or __CPROVER_assume(state->bytesInBuffer <= 16) to resolve this issue?
image
image

@qinheping
Copy link
Collaborator

qinheping commented Jul 2, 2024

While we are fixing #8361, you may want to use memset in SymCryptWipeAsm in the harness to avoid quantifiers in nested loops as a workaround.

@QinyuanWu
Copy link
Author

I made the following modification to Md2:

  • use memset to set buffer to zero in SymCryptWipeAsm
    image
  • correct loop invariant expression
    image
  • remove constrain for cbData(unbounded)
    image
    I still have the following errors and the coverage did not change:
    image
    image
    image
    image
    image
    Am I still missing something?

@QinyuanWu
Copy link
Author

My other question is for nested loops, if the outer loop has loop contracts, does the inner loops must also use loop contract?

@qinheping
Copy link
Collaborator

In the first error report, the first failure tells that the assign target j is missing on line 188. You need __CPROVER_assigns(t, j, k, __CPROVER_object_whole(pChain->X)).

In the last image, line 36-line54 are red because bytesInBuffser is 0 in this harness and the condition on line 32 is hence false. The error on line 63 looks like a real issue to me. Could you provide the error message for line 63?

My other question is for nested loops, if the outer loop has loop contracts, does the inner loops must also use loop contract?

Yes, the inner loops must also use loop contracts.

@QinyuanWu
Copy link
Author

All the errors are listed above, and somehow there's not an error associated with line 63. What are the potential causes of other errors in the report? Or are these errors that are subsequent results of the first one?

@QinyuanWu
Copy link
Author

QinyuanWu commented Jul 2, 2024

So I added three more loop contracts for Md2 which I believe covers all major long loops:

  1. Outer while loop in SymCryptMd2AppendBlocks
    image
  2. Two while loops in SymCryptXorBytes:
    image
    Could you help me check if these __CPROVER clauses makes sense for evaluating memory safety and if any invariants should be added or modified? When I run the proof the solver crashes first with the too many addressed objects.
    image
    I then increased object bits to 16 but the solver gets stuck and eventually killed. I tried for both SAT and SMT solvers.
    image

@qinheping
Copy link
Collaborator

For 1, the assigns clauses of a outer loop should include all assigns of the inner loops.
For 2, I will try to reproduce the error in my machine to see what is the error.

@QinyuanWu
Copy link
Author

I found that if the variable is declared inside the loop(e.g. for the outer while loop in 1, variable L is declared inside the loop), adding it to the assign clause will cause a conversion error:
image
Does that mean for these variables, even they would be included in the inner loop assign clauses, it's okey to not include the on the outer loop?

@remi-delmas-3000
Copy link
Collaborator

remi-delmas-3000 commented Jul 8, 2024

Hi, the assigns clause must only mention locations that are declared externally with respect to the loop, loop-locals are always considered assignable by the loop.

In the example below,

  • the assigns clause of loop 2 declares j and array as targets because the inner loop has side effects on both and j and array are external to loop 2
  • the assigns clause of the loop 1 declares i and array as targets because loop has side effects on both, but it does not declare j because j is local to loop 1.

Since loops are nested, the outer loop has at least all the side effect of the inner loop.

int array[16] = { {0} }; // external to both loop 1 and loop 2
int i = 0; // external to loop 1 only

// loop1
while( i < 4 )
// declares targets that are assigned and external to loop 1
__CPROVER_assigns(i, __CPROVER_object_whole(array)) 
{
   int j = 0; // local to loop 1, external to loop 2
   // loop2
   while (j < 4)
   // declares targets that are assigned and external to loop 2
   __CPROVER_assigns(j, __CPROVER_object_whole(array))
     {
      array[i*MAX + j];
      j++;
   }
   i++;
}

With for loops, variables declared in the for-clause are considered external to the loop:

for(size_t i; i < MAX ; i++)
__CPROVER_assigns(i,...)
{
  body();
}

Because the for is viewed as a while :

size_t i; 
while (i < MAX)
__CPROVER_assigns(i,...)
{
  body();
   i++;
}

and i "survives" between loop iterations.

There are other situations like this one that may seem strange : the side effect on *result is not considered part of the loop (because we identify loop at the CFG level and the instructions in the if are only ever executed once, and are not part of the connected component that defines the loop). However it is still part of the side effects of the function.

bool find_even(int *array, size_t size, size_t *result)
__CPROVER_requires(0 < size && size < 100000 && __CPROVER_is_fresh(array, size*sizeof(int)))
__CPROVER_requires(__CPROVER_is_fresh(result, sizeof(size_t)))
__CPROVER_assigns(*result)
{
  size_t i = 0;
  while(i < size)
  __CPROVER_assigns(i)
  __CPROVER_loop_invariant(i <= size)
  {
    // the if-condition evaluation is part of the loop 
     if (array[i]%2 == 0)
     {
       // this is not part of the loop
       *result = i;
       return true;
    }
    i++;
  }
  return false;
}

Hoping this helps.

@QinyuanWu
Copy link
Author

QinyuanWu commented Jul 9, 2024

I don't have any quantifiers in my loop contracts and when I used the --smt2 backend to solve the SymCryptMd2 proof I get the following errors:
image
but with the same goto program when I switched to the default SAT solver I don't get errors and wind up with just a unwind failure:
image
image

@qinheping
Copy link
Collaborator

ERROR means that the SMT return with error but the error message is not included in the image. So I am not sure what went wrong in the proof with SMT2.

For the unwinding error, could you try to remove the global unwinding flag --unwind 1? The assigns inclusion checks contain loops and need to unwind the loop for a constant number of time.

@QinyuanWu
Copy link
Author

For SAT, without --unwind 1 the solver gets killed after running about three minutes. I'm running on WSL ubuntu with 60GB memory.
image
image

@QinyuanWu
Copy link
Author

QinyuanWu commented Jul 11, 2024

@rod-chapman I was aware that the SMT solver had some issues with the quantifier so I tried to replicate your zero_slice example with the SAT solver by removing the forall quantifier:

//playground.c
#include <stdio.h>
#include <stdint.h>

void array_wipe(size_t len, uint8_t * array)
__CPROVER_requires(__CPROVER_is_fresh(array, len))
__CPROVER_assigns(__CPROVER_object_upto(array, len))
{ 
  __CPROVER_assume(array != NULL);

  size_t i;
  for (i = 0; i < len; i++)
  __CPROVER_assigns(i, __CPROVER_object_upto(array, len))
  __CPROVER_loop_invariant(i >= 0 && i <= len)
  {
      array[i] = 0; //set all array indices to 0
  }
}

but I got the following failures and unknowns:
image
image
Command I used:

goto-cc playground.c --function array_wipe
goto-instrument --dfcc array_wipe --apply-loop-contracts a.out sat.out
cbmc sat.out

Also an obversation is that without the __CPROVER_assume(array != NULL); there are a lot more failures and unknowns(see below), but I didn't see the assume clause in the zero_slice example.
image
image

@QinyuanWu
Copy link
Author

@qinheping Changes for SymCryptMd2 has been pushed, including the latest goto program and cbmc result. We have the following failures:

[SymCryptMd2AppendBlocks.loop_invariant_step.9] line 160 Check invariant after step for loop SymCryptMd2AppendBlocks.3: FAILURE
[SymCryptMd2AppendBlocks.precondition_instance.4] line 170 memcpy src/dst overlap: FAILURE
[SymCryptMd2AppendBlocks.precondition_instance.5] line 170 memcpy source region readable: FAILURE
[SymCryptMd2AppendBlocks.loop_invariant_base.9] line 197 Check invariant before entry for loop SymCryptMd2AppendBlocks.1: FAILURE
[SymCryptMd2AppendBlocks.loop_invariant_base.18] line 197 Check invariant before entry for loop SymCryptMd2AppendBlocks.1: FAILURE
[SymCryptMd2AppendBlocks.array_bounds.19] line 202 array 'SymCryptMd2STable' upper bound in SymCryptMd2STable[(signed long int)t]: FAILURE
[SymCryptMd2AppendBlocks.array_bounds.42] line 202 array 'SymCryptMd2STable' upper bound in SymCryptMd2STable[(signed long int)t]: FAILURE
[memcpy.pointer_arithmetic.71] line 33 pointer relation: pointer outside object bounds in (const char *)(const void *)pbData: FAILURE

@QinyuanWu
Copy link
Author

QinyuanWu commented Jul 12, 2024

Another issue for the make pipeline is that although I've specified CBMC_OBJECT_BITS in the makefile but it did not get translated into the --object-bits flag during the pipeline so the step fails. The --unwind flag is also missing.

[14/16] SymCryptMd2: checking safety properties                                                                                                                    FAILED: /home/jiggly/SymCrypt-CBMC/CBMC/proofs/SymCryptMd2/logs/result.xml /tmp/litani/runs/b68774f7-53d7-40d9-a60e-2ebbde6cc678/status/3bc78467-10f9-4ad0-8fe4-e60b42f060dc.json 

/usr/libexec/litani/litani exec --command 'cbmc  --flush --unwinding-assertions  --bounds-check           --trace --xml-ui /home/jiggly/SymCrypt-CBMC/CBMC/proofs/SymCryptMd2/gotos/SymCryptMd2_harness.goto' --pipeline-name SymCryptMd2 --ci-stage test --job-id 3bc78467-10f9-4ad0-8fe4-e60b42f060dc --stdout-file /home/jiggly/SymCrypt-CBMC/CBMC/proofs/SymCryptMd2/logs/result.xml --stderr-file /home/jiggly/SymCrypt-CBMC/CBMC/proofs/SymCryptMd2/logs/result-err-log.txt --description 'SymCryptMd2: checking safety properties' --timeout 21600 --status-file /tmp/litani/runs/b68774f7-53d7-40d9-a60e-2ebbde6cc678/status/3bc78467-10f9-4ad0-8fe4-e60b42f060dc.json --profile-memory-interval 10 --inputs /home/jiggly/SymCrypt-CBMC/CBMC/proofs/SymCryptMd2/gotos/SymCryptMd2_harness.goto --outputs /home/jiggly/SymCrypt-CBMC/CBMC/proofs/SymCryptMd2/logs/result.xml --ignore-returns 10 --tags 'stats-group:safety checks'

Makefile for SymCryptMd2

@QinyuanWu
Copy link
Author

QinyuanWu commented Jul 12, 2024

I worked with the SymCrypt developer and we've made some changes to reduce the failures.

For line 160 Check invariant after step for loop SymCryptMd2AppendBlocks.3, CBMC complains about __CPROVER_loop_invariant( __CPROVER_POINTER_OFFSET(pbData)+ cbData == __CPROVER_POINTER_OFFSET(__CPROVER_loop_entry(pbData))+ __CPROVER_loop_entry(cbData)) and through the trace we suspect that pbData+cbData might have an overflow and cause the invariant to not hold so we commented out this invariant clause.

For line 197 Check invariant before entry for loop SymCryptMd2AppendBlocks.1 and line 202 array 'SymCryptMd2STable' upper bound in SymCryptMd2STable[(signed long int)t], we realized that the invariant t < 256 should be specified in the outer loop so that it will not be havoc to an arbitrary value >= 256.
With these changes we are able to reduce the failures to these three:

[SymCryptMd2AppendBlocks.precondition_instance.4] line 171 memcpy src/dst overlap: FAILURE
[SymCryptMd2AppendBlocks.precondition_instance.5] line 171 memcpy source region readable: FAILURE
[memcpy.pointer_arithmetic.27] line 33 pointer relation: pointer outside object bounds in (const char *)(const void *)pbData: FAILURE

I tried adding __CPROVER_requires(__CPROVER_is_fresh(pbData, cbData) && __CPROVER_is_fresh(pChain, sizeof(* pChain))) to the function but the same failures persists.
The corresponding goto program has been updated and the new results and traces are in here.

@qinheping
Copy link
Collaborator

I think the missing part in the loop invariants is __CPROVER_loop_invariant(cbData <= __CPROVER_loop_entry( cbData )) saying that cbData decreases during the execution of the loop.

I used the following loop contracts for the outer while-loop (line 161 in md2.c),

    __CPROVER_assigns(t, j, k, cbData, pbData, __CPROVER_object_whole(pChain))
    __CPROVER_loop_invariant(cbData <= __CPROVER_loop_entry( cbData ))
    __CPROVER_loop_invariant( cbData % 16 == __CPROVER_loop_entry( cbData ) % 16 && __CPROVER_same_object(pbData, __CPROVER_loop_entry(pbData)))
    __CPROVER_loop_invariant( __CPROVER_POINTER_OFFSET(pbData)+ cbData == __CPROVER_POINTER_OFFSET(__CPROVER_loop_entry(pbData))+ __CPROVER_loop_entry(cbData))
    __CPROVER_decreases( cbData )

and run CBMC with the flag

cbmc gotos/SymCryptMd2_harness.goto --unwinding-assertions --unwind 50 --object-bits 9

. I got the result

** 0 of 2113 failed (1 iterations)
VERIFICATION SUCCESSFUL

@QinyuanWu
Copy link
Author

Thank you @qinheping! I can confirm that by adding __CPROVER_loop_invariant(cbData <= __CPROVER_loop_entry( cbData )) CBMC is able to solve the proof with an unbounded harness with --bounds-check --memory-leak-check --memory-cleanup-check --pointer-check --unwind 50 --object-bits 9. The entire pipeline(from generating goto to report) takes about ~5min.

I do wonder since the decreases clause is already saying that cbData decreases on every iteration, why do we still explicitly need the invariant clause to make it work?

@qinheping
Copy link
Collaborator

Thank you @qinheping! I can confirm that by adding __CPROVER_loop_invariant(cbData <= __CPROVER_loop_entry( cbData )) CBMC is able to solve the proof with an unbounded harness with --bounds-check --memory-leak-check --memory-cleanup-check --pointer-check --unwind 50 --object-bits 9. The entire pipeline(from generating goto to report) takes about ~5min.

I do wonder since the decreases clause is already saying that cbData decreases on every iteration, why do we still explicitly need the invariant clause to make it work?

Decreases clauses are used to prove the termination of the loops, and don't restrict the program state of the inductive step.

To be precise, the loop invariant __CPROVER_loop_invariant(cbData <= __CPROVER_loop_entry( cbData )) saying cbData not greater than its loop entry value instead of cbData decreasing. It is required to make sure that cbData is not out of the bound, and hence the offset of pbData is also not out of the bound deduced from __CPROVER_POINTER_OFFSET(pbData)+ cbData == __CPROVER_POINTER_OFFSET(__CPROVER_loop_entry(pbData))+ __CPROVER_loop_entry(cbData).

@samuel-lee-msft
Copy link

To be precise, the loop invariant __CPROVER_loop_invariant(cbData <= __CPROVER_loop_entry( cbData )) saying cbData not greater than its loop entry value instead of cbData decreasing

I guess the question is why does
__CPROVER_decreases( expr )
not imply
__CPROVER_loop_invariant( (expr) <= __CPROVER_loop_entry( expr ) )
internally in CBMC?
It seems to be a fairly obvious implication from the perspective of someone writing the annotations.

Is automatically adding this invariant in all cases costly?

@qinheping
Copy link
Collaborator

qinheping commented Jul 17, 2024

Is automatically adding this invariant in all cases costly?

No. It is not hard to automatically add this predicate into loop invariants.
I think it is a good idea but my concern here is that it will make invariants depend on the correctness of the decreases clauses and may make users confused about what clauses go wrong when CBMC report that loop invariants are not inductive. We will discuss more and consider to apply this rule. Thank you!

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

No branches or pull requests

5 participants