gh-libtom/libtomcrypt
Watch
1
0
Fork
You've already forked libtomcrypt
 0
Code Issues 42 Pull requests 17 Releases 9 Wiki Activity

Make base64_decode relaxed mode less relaxed #365

Merged
karel-m merged 6 commits from pr/base64-decode-less-relaxed into develop 2018-03-27 16:18:16 -05:00
Conversation 16 Commits 6 Files changed 4 +158 -52
karel-m commented 2018-03-25 12:48:07 -05:00 (Migrated from github.com)

Checklist

  • documentation is added or updated
  • tests are added or updated
### Checklist * [x] documentation is added or updated * [x] tests are added or updated
karel-m commented 2018-03-25 15:39:57 -05:00 (Migrated from github.com)

@sjaeckel wrote off-list:
I think we should try to keep this super relaxed mode if possible, as that's the one that is compatible with the old 1.17 behavior, right?

IMO the current super relaxed mode may be dangerous when you are decoding data from external sources (e.g. from a user request) because we are "able" to decode nearly any invalid input data. I am open to discussion what other characters should be ignored in relaxed mode.

> @sjaeckel wrote off-list: > I think we should try to keep this super relaxed mode if possible, as that's the one that is compatible with the old 1.17 behavior, right? IMO the current super relaxed mode may be dangerous when you are decoding data from external sources (e.g. from a user request) because we are "able" to decode nearly any invalid input data. I am open to discussion what other characters should be ignored in relaxed mode.
sjaeckel commented 2018-03-26 10:15:34 -05:00 (Migrated from github.com)

IMO the current super relaxed mode may be dangerous when you are decoding data from external sources (e.g. from a user request)...

I fully agree! ... and that's also what is said in RFC 4648 ...

But I also think that we shouldn't break existing code or at least give the possibility to enable the old behavior if required... so we've several options I guess...

  1. add a new API function base64_insane_decode() and fix the original
  2. add a new API function base64_sane_decode() and keep the original
  3. add a way to set the level of sanity checking...
    a. globally via a global
    b. globally via base64_set_decode_sanity()
    c. on the API level via exporting the sanity parameter
    d. at compile time
  4. or a combination of some of the above or something else I didn't think of?
> IMO the current super relaxed mode may be dangerous when you are decoding data from external sources (e.g. from a user request)... I fully agree! ... and that's also what is said in [RFC 4648](https://tools.ietf.org/html/rfc4648#section-12) ... But I also think that we shouldn't break existing code or at least give the possibility to enable the old behavior if required... so we've several options I guess... 1. add a new API function `base64_insane_decode()` and fix the original 1. add a new API function `base64_sane_decode()` and keep the original 1. add a way to set the level of sanity checking... a. globally via a global b. globally via `base64_set_decode_sanity()` c. on the API level via exporting the sanity parameter d. at compile time 1. or a combination of some of the above or something else I didn't think of?
karel-m commented 2018-03-26 14:08:48 -05:00 (Migrated from github.com)

From the options above I prefer base64_sane_decode() approach

From the options above I prefer `base64_sane_decode()` approach
sjaeckel commented 2018-03-26 14:16:27 -05:00 (Migrated from github.com)

From the options above I prefer base64_sane_decode() approach

👍

base64_sane_decode() shall it be

so we have now 3 possible values enum(insane, relaxed, strict) where the default base64_decode() is insane :D

> From the options above I prefer `base64_sane_decode()` approach :+1: `base64_sane_decode()` shall it be so we have now 3 possible values `enum(insane, relaxed, strict)` where the default `base64_decode()` is `insane` :D
karel-m commented 2018-03-26 15:37:23 -05:00 (Migrated from github.com)

base64_sane_decode or base64_relaxed_decode?

`base64_sane_decode` or `base64_relaxed_decode`?
sjaeckel commented 2018-03-26 16:15:52 -05:00 (Migrated from github.com)

I think base64_sane_decode() is more appropriate as base64_relaxed_decode() is basically lying because base64_decode() is more relaxed...

I think `base64_sane_decode()` is more appropriate as `base64_relaxed_decode()` is basically lying because `base64_decode()` is more relaxed...
sjaeckel (Migrated from github.com) reviewed 2018-03-26 16:19:18 -05:00
src/misc/base64/base64_decode.c
@ -89,20 +95,29 @@ static int _base64_decode_internal(const char *in, unsigned long inlen,
sjaeckel (Migrated from github.com) commented 2018-03-26 16:18:38 -05:00

can you please add parentheses?

can you please add parentheses?
src/misc/base64/base64_decode.c
@ -118,7 +133,7 @@ static int _base64_decode_internal(const char *in, unsigned long inlen,
sjaeckel (Migrated from github.com) commented 2018-03-26 16:18:53 -05:00

here as well please

here as well please
karel-m commented 2018-03-27 04:22:55 -05:00 (Migrated from github.com)

@sjaeckel please review again

@sjaeckel please review again
sjaeckel (Migrated from github.com) requested changes 2018-03-27 06:39:00 -05:00
sjaeckel (Migrated from github.com) left a comment

👍

:+1:
doc/crypt.tex
sjaeckel (Migrated from github.com) commented 2018-03-27 06:37:54 -05:00

should we add the URL variants to the index?

should we add the URL variants to the index?
src/misc/base64/base64_decode.c
@ -156,11 +171,25 @@ int base64_strict_decode(const char *in, unsigned long inlen,
{
sjaeckel (Migrated from github.com) commented 2018-03-27 06:19:10 -05:00

Probably this (and the URL variant) comment should be changed?

Probably this (and the URL variant) comment should be changed?
tests/base64_test.c
@ -62,2 +71,4 @@
{"\nvuiS\rPKIl8P\niR5O-rC4\tz9_xTQKZ0=\n", relaxed},
{"vuiSPKIl8PiR5O-rC4z9_xTQK", invalid},
};
sjaeckel (Migrated from github.com) commented 2018-03-27 06:19:39 -05:00

👍

:+1:
tests/base64_test.c
@ -79,0 +106,4 @@
DO(base64url_encode(tmp, l1, out, &l2));
DO(do_compare_testvector(out, l2, url_cases[x].s, strlen(url_cases[x].s), "base64url_encode", x));
}
if(x == 1) {
sjaeckel (Migrated from github.com) commented 2018-03-27 06:33:36 -05:00

I already thought about it while writing the last base16 testcase... should we add this?

diff --git a/tests/common.h b/tests/common.h
index 1ba8820..f63e26b 100644
--- a/tests/common.h
+++ b/tests/common.h
@@ -18,2 +18,3 @@ extern prng_state yarrow_prng;
 #define DOX(x, str) do { fprintf(stderr, "%s - %s:\n", #x, (str)); run_cmd((x), __LINE__, __FILE__, #x, (str)); } while (0)
+#define SHOULD_FAIL(x) do { fprintf(stderr, "%s:\n", #x); run_cmd((x) != CRYPT_OK ? CRYPT_OK : CRYPT_FAIL_TESTVECTOR, __LINE__, __FILE__, #x, NULL); } while (0)
 #else
@@ -21,2 +22,3 @@ extern prng_state yarrow_prng;
 #define DOX(x, str) do { run_cmd((x), __LINE__, __FILE__, #x, (str)); } while (0)
+#define SHOULD_FAIL(x) do { run_cmd((x) != CRYPT_OK ? CRYPT_OK : CRYPT_FAIL_TESTVECTOR, __LINE__, __FILE__, #x, NULL); } while (0)
 #endif
I already thought about it while writing the last base16 testcase... should we add this? ``` diff --git a/tests/common.h b/tests/common.h index 1ba8820..f63e26b 100644 --- a/tests/common.h +++ b/tests/common.h @@ -18,2 +18,3 @@ extern prng_state yarrow_prng; #define DOX(x, str) do { fprintf(stderr, "%s - %s:\n", #x, (str)); run_cmd((x), __LINE__, __FILE__, #x, (str)); } while (0) +#define SHOULD_FAIL(x) do { fprintf(stderr, "%s:\n", #x); run_cmd((x) != CRYPT_OK ? CRYPT_OK : CRYPT_FAIL_TESTVECTOR, __LINE__, __FILE__, #x, NULL); } while (0) #else @@ -21,2 +22,3 @@ extern prng_state yarrow_prng; #define DOX(x, str) do { run_cmd((x), __LINE__, __FILE__, #x, (str)); } while (0) +#define SHOULD_FAIL(x) do { run_cmd((x) != CRYPT_OK ? CRYPT_OK : CRYPT_FAIL_TESTVECTOR, __LINE__, __FILE__, #x, NULL); } while (0) #endif ```
karel-m (Migrated from github.com) reviewed 2018-03-27 06:51:45 -05:00
tests/base64_test.c
@ -79,0 +106,4 @@
DO(base64url_encode(tmp, l1, out, &l2));
DO(do_compare_testvector(out, l2, url_cases[x].s, strlen(url_cases[x].s), "base64url_encode", x));
}
if(x == 1) {
karel-m (Migrated from github.com) commented 2018-03-27 06:51:45 -05:00

Maybe, but perhaps as SHOULD_FAIL(x, expected_retval)

Maybe, but perhaps as `SHOULD_FAIL(x, expected_retval)`
karel-m (Migrated from github.com) reviewed 2018-03-27 06:51:59 -05:00
src/misc/base64/base64_decode.c
@ -156,11 +171,25 @@ int base64_strict_decode(const char *in, unsigned long inlen,
{
karel-m (Migrated from github.com) commented 2018-03-27 06:51:59 -05:00

done

done
karel-m (Migrated from github.com) reviewed 2018-03-27 07:02:31 -05:00
doc/crypt.tex
karel-m (Migrated from github.com) commented 2018-03-27 07:02:31 -05:00

done

done
sjaeckel (Migrated from github.com) reviewed 2018-03-27 07:19:36 -05:00
tests/base64_test.c
@ -79,0 +106,4 @@
DO(base64url_encode(tmp, l1, out, &l2));
DO(do_compare_testvector(out, l2, url_cases[x].s, strlen(url_cases[x].s), "base64url_encode", x));
}
if(x == 1) {
sjaeckel (Migrated from github.com) commented 2018-03-27 07:19:36 -05:00

I also thought about EXPECT(x, e) but in my opinion that's another use-case.
Most cases where we do negative tests shouldn't care about the error-code returned, it should just be ensured that the result is not OK.

I also thought about `EXPECT(x, e)` but in my opinion that's another use-case. Most cases where we do negative tests shouldn't care about the error-code returned, it should just be ensured that the result is not OK.
sjaeckel (Migrated from github.com) reviewed 2018-03-27 07:21:21 -05:00
src/misc/base64/base64_decode.c
sjaeckel (Migrated from github.com) commented 2018-03-27 07:21:21 -05:00

:) well put!

:) well put!
karel-m commented 2018-03-27 14:20:26 -05:00 (Migrated from github.com)

Is it ok?

Is it ok?
sjaeckel (Migrated from github.com) approved these changes 2018-03-27 14:25:13 -05:00
Sign in to join this conversation.
Reviewers
No reviewers
sjaeckel
Labels
Clear labels   
bug

   
duplicate

   
enhancement

   
feature

   
help wanted

   
improve API

   
invalid

   
needs documentation

   
not-planned
This ticket is not planned to be implemented and/or fixed from the core team.

  
rtfm

   
stale

   
support

   
wishlist
No labels
bug
duplicate
enhancement
feature
help wanted
improve API
invalid
needs documentation
not-planned
rtfm
stale
support
wishlist
Milestone
Clear milestone
No items
No milestone
next
Assignees
Clear assignees
No assignees
1 participant
Notifications
Due date
The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set

Reference
gh-libtom/libtomcrypt!365
No description provided.