initialize 'flags' etc. to invalid values before trying to decode #298

2017-09-24T06:13:57-05:00

sjaeckel commented

2017-09-24 06:13:57 -05:00

(Migrated from github.com)

I'm not sure if that's really required, but I'm just putting this here to get your opinion.

karel-m (Migrated from github.com) reviewed

2017-09-24 09:07:58 -05:00

src/pk/dh/dh_import.c

					
				@ -71,2 +71,4 @@

				         }

				      }

				      else {

				         err = CRYPT_INVALID_PACKET;

Although unrelated to the main idea of this PR I think that it would be better to:

mp_set_int(key->x, 0);

instead of:

mp_clear(key->x);
key->x = NULL;

Although unrelated to the main idea of this PR I think that it would be better to: ``` mp_set_int(key->x, 0); ``` instead of: ``` mp_clear(key->x); key->x = NULL; ```

karel-m (Migrated from github.com) approved these changes

2017-09-24 09:17:32 -05:00

karel-m (Migrated from github.com) left a comment

👍 just consider my comment on dh_import.c

👍 just consider my comment on `dh_import.c`

sjaeckel (Migrated from github.com) reviewed

2017-09-24 12:50:13 -05:00

src/pk/dh/dh_import.c

					
				@ -71,2 +71,4 @@

				         }

				      }

				      else {

				         err = CRYPT_INVALID_PACKET;

uhm, why?

karel-m (Migrated from github.com) reviewed

2017-09-24 13:10:27 -05:00

src/pk/dh/dh_import.c

					
				@ -71,2 +71,4 @@

				         }

				      }

				      else {

				         err = CRYPT_INVALID_PACKET;

It might got a bit tricky as we use *_multi functions where we use NULL as an end of args marker. Like in dh_free:

mp_cleanup_multi(&key->prime, &key->base, &key->y, &key->x, NULL);

which means that setting key->x to NULL is fine but only till somebody will add an extra stuff like ..., &key->x, &key->extra, NULL); (which can easily happen as there is no comment in dh_free that key->x must be the last arg of mp_cleanup_multi).

We do not use this approach anywhere else and I am not sure if it is so useful.

It might got a bit tricky as we use `*_multi` functions where we use NULL as an end of args marker. Like in `dh_free`: ```C mp_cleanup_multi(&key->prime, &key->base, &key->y, &key->x, NULL); ``` which means that setting `key->x` to NULL is fine but only till somebody will add an extra stuff like `..., &key->x, &key->extra, NULL);` (which can easily happen as there is no comment in `dh_free` that `key->x` must be the last arg of `mp_cleanup_multi`). We do not use this approach anywhere else and I am not sure if it is so useful.

sjaeckel (Migrated from github.com) reviewed

2017-09-24 18:21:15 -05:00

src/pk/dh/dh_import.c

					
				@ -71,2 +71,4 @@

				         }

				      }

				      else {

				         err = CRYPT_INVALID_PACKET;

makes sense

sjaeckel (Migrated from github.com) reviewed

2017-09-25 11:27:59 -05:00

src/pk/dh/dh_import.c

					
				@ -71,2 +71,4 @@

				         }

				      }

				      else {

				         err = CRYPT_INVALID_PACKET;

I decided to apply the change directly on the release branch, but didn't merge this PR as I'm doubting its usefulness...

karel-m commented

2017-09-25 11:44:02 -05:00

(Migrated from github.com)

didn't merge this PR as I'm doubting its usefulness...

When I was looking at the changes in this PR the only scenario that come to my mind was a case when something deep in the ASN.1 stuff returns CRYPT_PK_INVALID_SIZE which we will falsely consider as OK/expected (like in case when there are extra input data at the end of parsing ASN.1 sequence).

But it can be handled for example by a new error code like CRYPT_SEQ_DATA_TOO_LONG which will be guaranteed just for the case "extra input data at the end of parsing ASN.1 sequence".

> didn't merge this PR as I'm doubting its usefulness... When I was looking at the changes in this PR the only scenario that come to my mind was a case when something deep in the ASN.1 stuff returns `CRYPT_PK_INVALID_SIZE` which we will falsely consider as OK/expected (like in case when there are extra input data at the end of parsing ASN.1 sequence). But it can be handled for example by a new error code like `CRYPT_SEQ_DATA_TOO_LONG` which will be guaranteed just for the case "extra input data at the end of parsing ASN.1 sequence".

karel-m commented

2017-09-25 14:45:40 -05:00

(Migrated from github.com)

The name CRYPT_INPUT_TOO_LONG sounds good 👍

The name `CRYPT_INPUT_TOO_LONG` sounds good 👍

karel-m commented

2017-09-27 02:14:03 -05:00

(Migrated from github.com)

I just want to confirm that I approve also the latest version of this PR.

@sjaeckel merge this depending on your plan of the next steps in the release process. IMO if there is a RC5 than I am for merging this into it. But it could also wait for the next.

I just want to confirm that I approve also the latest version of this PR. @sjaeckel merge this depending on your plan of the next steps in the release process. IMO if there is a RC5 than I am for merging this into it. But it could also wait for the `next`.

Sign in to join this conversation.

No reviewers

karel-m