Merge pull request #8027 from bplu4t2f/base64-3

Fix additional base64 decoder edge cases

core/encoding/base64/base64.odin

@@ -132,6 +132,7 @@ Decode_Error :: enum {
 	Invalid_Character,
 	Invalid_Padding,
 	Non_Canonical,
+	Malformed_Quantum,
 }
 
 // Decode_Option selects optional validation performed by the decode routines.
@@ -308,42 +309,53 @@ validate_strict_decode :: proc(data: string, dec_tbl: [256]i8, options: Decode_O
 			si -= 1
 			pad += 1
 		}
-		if pad > 2 {
-			return .Invalid_Padding
-		}
 		for j in 0 ..< si {
 			if data[j] == PADDING {
+				// Interior padding, e.g. QQ=Q
 				return .Invalid_Padding
 			}
 		}
 	}
 
+	// `si` is the amount of meaningful data, excluding trailing padding.
+
+	// Number of non-padding characters in the last quantum (e.g. 2 for "QQ==", 3 for "QUI=").
 	rem := si % 4
-	if .No_Padding in options && rem == 1 {
-		return .Invalid_Padding
+	if rem == 1 {
+		// "QUJDQ"  ||  "QUJDQ==="
+		//  -> unsalvageable (this is not just missing padding or non-canonical).
+		return .Malformed_Quantum
 	}
 
 	if .Strict in options {
 		if .No_Padding not_in options {
 			if n % 4 != 0 {
+				// "QQ"  ||  "QUI"
+				//  -> MUST be "QQ=="  ||  "QUI="
 				return .Invalid_Padding
 			}
 
 			switch rem {
 			case 0:
 				if pad != 0 {
+					// "QUJD="  ||  "QUJD=="
+					//  -> MUST be "QUJD"
 					return .Invalid_Padding
 				}
 			case 2:
 				if pad != 2 {
+					// "QQ"  ||  "QQ="
+					//  -> MUST be "QQ=="
 					return .Invalid_Padding
 				}
 			case 3:
 				if pad != 1 {
+					// "QUI"  ||  "QUI=="
+					//  -> MUST be "QUI="
 					return .Invalid_Padding
 				}
 			case:
-				return .Invalid_Padding
+				unreachable()
 			}
 		}
 
@@ -354,6 +366,8 @@ validate_strict_decode :: proc(data: string, dec_tbl: [256]i8, options: Decode_O
 				return .Invalid_Character
 			}
 			if (c & 0x0f) != 0 {
+				// In the last quantum, the second encoded character would decode into
+				// non-zero bits in the second decoded byte, which is beyond the decoded range.
 				return .Non_Canonical
 			}
 		case 3:
@@ -362,6 +376,8 @@ validate_strict_decode :: proc(data: string, dec_tbl: [256]i8, options: Decode_O
 				return .Invalid_Character
 			}
 			if (c & 0x03) != 0 {
+				// In the last quantum, the third encoded character would decode into
+				// non-zero bits in the third decoded byte, which is beyond the decoded range.
 				return .Non_Canonical
 			}
 		}
@@ -370,72 +386,105 @@ validate_strict_decode :: proc(data: string, dec_tbl: [256]i8, options: Decode_O
 	return .None
 }
 
-decode :: proc(data: string, DEC_TBL := DEC_TABLE, dst: []byte = nil, allocator := context.allocator, options := Decode_Options{}) -> (decoded: []byte, err: Error) {
-	if derr := validate_strict_decode(data, DEC_TBL, options); derr != .None {
-		return nil, derr
+@(private)
+Decode_Info :: struct {
+	out_length: int,
+	trimmed_in: string,
+}
+
+@(private)
+decode_analyze :: proc(data: string, DEC_TBL := DEC_TABLE, options: Decode_Options) -> (info: Decode_Info, err: Decode_Error) {
+	err = validate_strict_decode(data, DEC_TBL, options)
+	if err != nil {
+		return
 	}
 
-	out_length := decoded_len(data)
-	if out_length == 0 {
+	// Calculate trimmed input length based on stripped trailing padding.
+	length_in := len(data)
+	for length_in > 0 && data[length_in - 1] == PADDING {
+		length_in -= 1
+	}
+
+	quanta := length_in >> 2
+	rem_in := length_in & 3
+	rem_out: int
+	switch rem_in {
+	case 0:
+	case 1:
+		err = .Malformed_Quantum
+		return
+	case 2: rem_out = 1
+	case 3: rem_out = 2
+	case:   unreachable()
+	}
+
+	info = {
+		out_length = quanta * 3 + rem_out,
+		trimmed_in = data[:length_in],
+	}
+	return
+}
+
+decode :: proc(data: string, DEC_TBL := DEC_TABLE, dst: []byte = nil, allocator := context.allocator, options := Decode_Options{}) -> (decoded: []byte, err: Error) {
+	decode_info := decode_analyze(data, DEC_TBL, options) or_return
+
+	if decode_info.out_length == 0 {
 		return nil, nil
 	}
 
 	buf: []byte
-	if buf, err = make([]byte, out_length, allocator); err != nil {
+	if buf, err = make([]byte, decode_info.out_length, allocator); err != nil {
 		return
 	}
 
-	decoded, err = decode_impl(buf, data, DEC_TBL)
+	decoded, err = decode_impl(buf, decode_info, DEC_TBL)
 	if err != nil {
 		delete(buf, allocator)
 	}
-	assert(err != nil || len(decoded) == out_length, "buffer unexpectedly resized, `decoded_len` was wrong")
+	assert(err != nil || len(decoded) == decode_info.out_length, "buffer unexpectedly resized, `analyze_decode` was wrong")
 
 	return
 }
 
 decode_into_buf :: proc(dst: []byte, data: string, DEC_TBL := DEC_TABLE, options := Decode_Options{}) -> (decoded: []byte, err: Error) {
-	if derr := validate_strict_decode(data, DEC_TBL, options); derr != .None {
-		return nil, derr
-	}
+	decode_info := decode_analyze(data, DEC_TBL, options) or_return
 
-	out_length := decoded_len(data)
-	if out_length == 0 {
+	if decode_info.out_length == 0 {
 		return
 	}
 
-	return decode_impl(dst, data, DEC_TBL)
+	return decode_impl(dst, decode_info, DEC_TBL)
 }
 
 decode_into :: proc(w: io.Writer, data: string, DEC_TBL := DEC_TABLE, options := Decode_Options{}) -> Error {
-	if derr := validate_strict_decode(data, DEC_TBL, options); derr != .None {
-		return derr
-	}
+	decode_info := decode_analyze(data, DEC_TBL, options) or_return
 
-	_, err := decode_impl(w, data, DEC_TBL)
+	_, err := decode_impl(w, decode_info, DEC_TBL)
 	return err
 }
 
 @(private)
-decode_impl :: proc(dst: $T, data: string, DEC_TBL := DEC_TABLE) -> ([]byte, Error) where T == io.Writer || T == []byte {
-	length := decoded_len(data)
+decode_impl :: proc(dst: $T, info: Decode_Info, DEC_TBL := DEC_TABLE) -> ([]byte, Error) where T == io.Writer || T == []byte {
+	dst_cursor: int
 	when T == []byte {
-		if len(dst) < length {
+		if len(dst) < info.out_length {
 			return nil, io.Error.Short_Buffer
 		}
-		off: int
 	} else {
-		if length == 0 {
+		if info.out_length == 0 {
 			return nil, nil
 		}
 		buf: [3]byte
 	}
 
+	// Note: decode_analyze handles removing padding.
+	data := info.trimmed_in
+
 	c0, c1, c2, c3: int
 	d0, d1, d2, d3: i8
 	b0, b1, b2: int
-	i, j: int
-	for ; j + 3 <= length; i, j = i + 4, j + 3 {
+	i: int
+	for ; i + 4 <= len(data); i += 4 {
 		#no_bounds_check {
 			d0 = DEC_TBL[data[i]]
 			d1 = DEC_TBL[data[i + 1]]
@@ -443,7 +492,17 @@ decode_impl :: proc(dst: $T, data: string, DEC_TBL := DEC_TABLE) -> ([]byte, Err
 			d3 = DEC_TBL[data[i + 3]]
 
 			if intrinsics.unlikely((d0 | d1 | d2 | d3) & ~i8(0x3f) != 0) {
-				return nil, Decode_Error.Invalid_Character
+				// To be consistent with validate_strict_decode, this will return
+				// Invalid_Padding if the input string contains invalid padding
+				// characters, but would otherwise be valid.
+				if (data[i + 0] == PADDING || d0 & ~i8(0x3f) == 0) &&
+				   (data[i + 1] == PADDING || d1 & ~i8(0x3f) == 0) &&
+				   (data[i + 2] == PADDING || d2 & ~i8(0x3f) == 0) &&
+				   (data[i + 3] == PADDING || d3 & ~i8(0x3f) == 0) {
+					return nil, .Invalid_Padding
+				} else {
+					return nil, .Invalid_Character
+				}
 			}
 
 			c0, c1, c2, c3 = int(d0), int(d1), int(d2), int(d3)
@@ -453,10 +512,10 @@ decode_impl :: proc(dst: $T, data: string, DEC_TBL := DEC_TABLE) -> ([]byte, Err
 			b2 = (c2 << 6) | c3
 
 			when T == []byte {
-				dst[off+0] = byte(b0)
-				dst[off+1] = byte(b1)
-				dst[off+2] = byte(b2)
-				off += 3
+				dst[dst_cursor+0] = byte(b0)
+				dst[dst_cursor+1] = byte(b1)
+				dst[dst_cursor+2] = byte(b2)
+				dst_cursor += 3
 			} else {
 				buf[0] = byte(b0)
 				buf[1] = byte(b1)
@@ -468,21 +527,32 @@ decode_impl :: proc(dst: $T, data: string, DEC_TBL := DEC_TABLE) -> ([]byte, Err
 			if _, err := io.write_full(dst, buf[:]); err != .None {
 				return nil, err
 			}
+			dst_cursor += 3
 		}
 	}
 
-	rest := length - j
-	if rest > 0 {
+	rest_in := len(data) - i
+	if rest_in > 0 {
+		when T == io.Writer {
+			rest_out: int
+		}
+
 		#no_bounds_check {
-			// Note: decoded_len handles removing padding.
-			d0 = DEC_TBL[data[i]]
-			d1 = DEC_TBL[data[i + 1]]
-			if d2 = 0; rest == 2 {
-				d2 = DEC_TBL[data[i + 2]]
-			}
+			d0 = DEC_TBL[data[i + 0]]
+			d1 = DEC_TBL[data[i + 1]] if rest_in >= 2 else 0
+			d2 = DEC_TBL[data[i + 2]] if rest_in >= 3 else 0
 
 			if intrinsics.unlikely((d0 | d1 | d2) & ~i8(0x3f) != 0) {
-				return nil, Decode_Error.Invalid_Character
+				// To be consistent with validate_strict_decode, this will return
+				// Invalid_Padding if the input string contains invalid padding
+				// characters, but would otherwise be valid.
+				if (               (data[i + 0] == PADDING || d0 & ~i8(0x3f) == 0)) &&
+				   (rest_in < 2 || (data[i + 1] == PADDING || d1 & ~i8(0x3f) == 0)) &&
+				   (rest_in < 3 || (data[i + 2] == PADDING || d2 & ~i8(0x3f) == 0)) {
+					return nil, .Invalid_Padding
+				} else {
+					return nil, .Invalid_Character
+				}
 			}
 
 			c0, c1, c2 = int(d0), int(d1), int(d2)
@@ -491,47 +561,56 @@ decode_impl :: proc(dst: $T, data: string, DEC_TBL := DEC_TABLE) -> ([]byte, Err
 			b1 = (c1 << 4) | (c2 >> 2)
 
 			when T == []byte {
-				switch rest {
-				case 2:
-					dst[off+1] = byte(b1)
-					fallthrough
+				switch rest_in {
 				case 1:
-					dst[off] = byte(b0)
+					// Last quantum is malformed:  "Q==="
+					return nil, .Malformed_Quantum
+				case 2:
+					dst[dst_cursor + 0] = byte(b0)
+					dst_cursor += 1
+				case 3:
+					dst[dst_cursor + 0] = byte(b0)
+					dst[dst_cursor + 1] = byte(b1)
+					dst_cursor += 2
 				}
 			} else {
-				buf[0] = byte(b0)
-				buf[1] = byte(b1)
+				switch rest_in {
+				case 1:
+					// Last quantum is malformed:  "Q==="
+					return nil, .Malformed_Quantum
+				case 2:
+					buf[0] = byte(b0)
+					rest_out = 1
+				case 3:
+					buf[0] = byte(b0)
+					buf[1] = byte(b1)
+					rest_out = 2
+				}
 			}
 		}
 
 		when T == io.Writer {
-			if _, err := io.write_full(dst, buf[:rest]); err != .None {
+			if _, err := io.write_full(dst, buf[:rest_out]); err != .None {
 				return nil, err
 			}
+			dst_cursor += rest_out
 		}
 	}
 
+	assert(dst_cursor == info.out_length)
+
 	when T == io.Writer {
 		return nil, nil
 	} else {
-		return dst[:length], nil
+		return dst[:dst_cursor], nil
 	}
 }
 
-decoded_len :: proc(data: string) -> int {
-	length := len(data)
-	if length == 0 {
-		return 0
-	}
-
-	padding: int
-	if data[length - 1] == PADDING {
-		if length > 1 && data[length - 2] == PADDING {
-			padding = 2
-		} else {
-			padding = 1
-		}
-	}
-
-	return max(0, ((length * 6) >> 3) - padding)
+decoded_len :: proc(data: string, DEC_TBL := DEC_TABLE, options: Decode_Options = {}) -> (n: int, err: Decode_Error) {
+	// NOTE(bplu4t2f):
+	//     It's not clear whether this is intended to be a public API.
+	//     There are no regression tests for it.
+	info := decode_analyze(data, DEC_TBL, options) or_return
+	n = info.out_length
+	return
 }

core/encoding/cbor/tags.odin

@@ -306,7 +306,12 @@ tag_base64_unmarshal :: proc(_: ^Tag_Implementation, d: Decoder, _: Tag_Number,
 	case reflect.Type_Info_String:
 		assert(t.encoding == .UTF_8)
 		if t.is_cstring {
-			length  := base64.decoded_len(bytes)
+			// Convert this using an auxiliary builder so we get a terminating null character.
+			length, derr := base64.decoded_len(bytes)
+			if derr != nil {
+				err = err_from_b64(derr)
+				return
+			}
 			builder := strings.builder_make(0, length+1)
 			b64_decode_into(strings.to_stream(&builder), bytes) or_return
 
@@ -347,9 +352,15 @@ tag_base64_unmarshal :: proc(_: ^Tag_Implementation, d: Decoder, _: Tag_Number,
 
 		if elem_base.id != byte { return _unsupported(v, hdr) }
 
-		if base64.decoded_len(bytes) > t.count { return _unsupported(v, hdr) }
+		decoded_length, derr := base64.decoded_len(bytes)
+		if derr != nil {
+			err = err_from_b64(derr)
+			return
+		}
+		if decoded_length > t.count { return _unsupported(v, hdr) }
 
 		slice := ([^]byte)(v.data)[:len(bytes)]
+		// TODO: base64 can decode directly into a byte buffer.
 		copy(slice, b64_decode(bytes) or_return)
 		return
 	}

tests/core/encoding/base64/base64.odin

@@ -126,7 +126,7 @@ test_strict_reject :: proc(t: ^testing.T) {
 		options: base64.Decode_Options,
 		err:     base64.Decode_Error,
 	} {
-		{"A", {.Strict}, .Invalid_Padding},
+		{"A", {.Strict}, .Malformed_Quantum},
 		{"AA", {.Strict}, .Invalid_Padding},
 		{"Zg", {.Strict}, .Invalid_Padding},
 		{"Zm8", {.Strict}, .Invalid_Padding},
@@ -140,7 +140,7 @@ test_strict_reject :: proc(t: ^testing.T) {
 		{"WvLTlMrX9NpYDQlEIFlnDB==", {.Strict}, .Non_Canonical},
 		{"Zg==", {.Strict, .No_Padding}, .Invalid_Padding},
 		{"Zm8=", {.Strict, .No_Padding}, .Invalid_Padding},
-		{"Z", {.Strict, .No_Padding}, .Invalid_Padding},
+		{"Z", {.Strict, .No_Padding}, .Malformed_Quantum},
 		{"QR", {.Strict, .No_Padding}, .Non_Canonical},
 		{"!!!!", {.Strict}, .Invalid_Character},
 		{"Zm9!", {.Strict}, .Invalid_Character},
@@ -467,7 +467,7 @@ test_rfc4648_and_go_vectors :: proc(t: ^testing.T) {
 }
 
 @(test)
-test_newline_rejected :: proc(t: ^testing.T) {
+test_newline_rejected_strict :: proc(t: ^testing.T) {
 	// Inputs from Go's TestNewLineCharacters. Go's decoder deliberately ignores
 	// "\r" and "\n" (MIME legacy); RFC 4648 section 3.3 says implementations
 	// MUST reject characters outside the alphabet, so Odin must not accept them.
@@ -487,21 +487,166 @@ test_newline_rejected :: proc(t: ^testing.T) {
 	}
 	for c in strict_cases {
 		decoded, err := base64.decode(c, options = {.Strict})
-		testing.expectf(t, err != nil, "strict decode(%q): expected rejection, got %q", c, decoded)
+		testing.expectf(t, err != nil, "strict decode(%q): expected rejection, got %q", c, err)
 		delete(decoded)
 	}
+}
 
-	// Lenient mode only scans the input span `decoded_len` implies, so bytes
-	// past the decoded tail are not always inspected (e.g. "c3VyZQ\n=="
-	// currently decodes to "sure"); interior ones are always seen.
+@(test)
+test_newline_rejected_lenient :: proc(t: ^testing.T) {
 	lenient_cases := []string{
+		"c3VyZQ\n==",
 		"c3VyZQ==\n",
 		"c3VyZQ=\n=",
 		"c3V\ryZ\rQ==",
 	}
 	for c in lenient_cases {
 		decoded, err := base64.decode(c)
-		testing.expectf(t, err == base64.Decode_Error.Invalid_Character, "lenient decode(%q): got %v, want Invalid_Character", c, err)
+		// NOTE(bplu4t2f):
+		//     Ideally we would like to see Invalid_Character for all of these, however, that
+		//     would imply an O(N) linear scan through the entire rest of the input data, even
+		//     though we already know for sure that it contains an error.
+		//     Ex.:  QQ==QQ==Q\nQQQQ==
+		//             ^     ^^^^^
+		//             |     this quantum is not even inspected.
+		//             interior padding; decoding failure is already set in stone.
+		//
+		//     Instead, the implementation bails out early on error.
+		//     That means that this can either yield Invalid_Padding or Invalid_Character, for now.
+		testing.expectf(t, err != nil, "lenient decode(%q): expected rejection, got %q", c, err)
 		testing.expect(t, decoded == nil)
 	}
 }
+
+@(test)
+reject_lenient_malformed_quantum_1 :: proc(t: ^testing.T) {
+	decoded, err := base64.decode("Q===")
+	testing.expect_value(t, err, .Malformed_Quantum)
+	testing.expect(t, decoded == nil)
+}
+
+@(test)
+reject_strict_malformed_quantum_1 :: proc(t: ^testing.T) {
+	decoded, err := base64.decode("Q===", options = { .Strict })
+	testing.expect_value(t, err, .Malformed_Quantum)
+	testing.expect(t, decoded == nil)
+}
+
+@(test)
+reject_lenient_malformed_quantum_2 :: proc(t: ^testing.T) {
+	decoded, err := base64.decode("QUJDQ===")
+	testing.expect_value(t, err, .Malformed_Quantum)
+	testing.expect(t, decoded == nil)
+}
+
+@(test)
+reject_strict_malformed_quantum_2 :: proc(t: ^testing.T) {
+	decoded, err := base64.decode("QUJDQ===", options = { .Strict })
+	testing.expect_value(t, err, .Malformed_Quantum)
+	testing.expect(t, decoded == nil)
+}
+
+@(test)
+reject_lenient_malformed_quantum_3 :: proc(t: ^testing.T) {
+	decoded, err := base64.decode("Q===QUJD")
+	// Debatable: The current implementation treats any interior padding as
+	// "Invalid_Padding", even though in this case an argument could
+	// be made for "Malformed_Quantum" instead.
+	testing.expect(t, err != nil)
+	testing.expect(t, decoded == nil)
+}
+
+@(test)
+reject_strict_malformed_quantum_3 :: proc(t: ^testing.T) {
+	decoded, err := base64.decode("Q===QUJD", options = { .Strict })
+	// Debatable: The current implementation treats any interior padding as
+	// "Invalid_Padding", even though in this case an argument could
+	// be made for "Malformed_Quantum" instead.
+	testing.expect(t, err != nil)
+	testing.expect(t, decoded == nil)
+}
+
+@(private)
+dynamic_array_stream_proc :: proc(stream_data: rawptr, mode: io.Stream_Mode, p: []byte, offset: i64, whence: io.Seek_From) -> (n: i64, err: io.Error) {
+	b := (^[dynamic]byte)(stream_data)
+	#partial switch mode {
+	case .Write:
+		written, err := append(b, ..p)
+		if err != nil {
+			return auto_cast written, .EOF
+		} else {
+			return auto_cast written, nil
+		}
+	case .Size:
+		n = i64(len(b))
+		return
+	case .Destroy:
+		delete(b^)
+		return
+	case .Query:
+		return io.query_utility({.Write, .Size, .Destroy, .Query})
+	}
+	return 0, .Unsupported
+}
+
+@(test)
+writer_path_malformed_tails :: proc(t: ^testing.T) {
+	backing := make([dynamic]byte)
+	defer delete(backing)
+	writer := io.Writer { procedure = dynamic_array_stream_proc, data = cast(rawptr)&backing }
+
+	err := base64.decode_into(writer, "QUJDQ")
+	testing.expect_value(t, err, .Malformed_Quantum)
+}
+
+@(test)
+invalid_padding_error_lenient :: proc(t: ^testing.T) {
+	decoded, err := base64.decode("A=AA")
+	testing.expect_value(t, err, .Invalid_Padding)
+	testing.expect(t, decoded == nil)
+}
+
+@(test)
+invalid_padding_error_strict :: proc(t: ^testing.T) {
+	decoded, err := base64.decode("A=AA", options = {.Strict})
+	testing.expect_value(t, err, .Invalid_Padding)
+	testing.expect(t, decoded == nil)
+}
+
+@(test)
+invalid_padding_error_end_lenient :: proc(t: ^testing.T) {
+	decoded, err := base64.decode("QUJDA=A")
+	testing.expect_value(t, err, .Invalid_Padding)
+	testing.expect(t, decoded == nil)
+}
+
+@(test)
+invalid_padding_error_end_strict :: proc(t: ^testing.T) {
+	decoded, err := base64.decode("QUJDA=A", options = {.Strict})
+	testing.expect_value(t, err, .Invalid_Padding)
+	testing.expect(t, decoded == nil)
+}
+
+@(test)
+lenient_ignore_excess_padding :: proc(t: ^testing.T) {
+	// Test different well-formed inputs with varying amount of padding,
+	// e.g.  "QQ====="
+	cases := []struct {
+		input: string,
+		output: string,
+	} {
+		{ input = "",      output = "",     },
+		{ input = "QQ",    output = "A",    },
+		{ input = "QUI",   output = "AB",   },
+		{ input = "QUJD",  output = "ABC",  },
+	}
+	for base in cases {
+		for num_padding in 1 ..= 8 {
+			s := strings.concatenate([]string { base.input, strings.repeat("=", num_padding, context.temp_allocator) }, context.temp_allocator)
+			decoded, err := base64.decode(s)
+			defer delete(decoded)
+			testing.expect_value(t, err, nil)
+			testing.expect_value(t, transmute(string)decoded, base.output)
+		}
+	}
+}

tests/issues/test_issue_7763/test_issue_7763.odin

@@ -47,7 +47,7 @@ test_issue_7763__3eq :: proc(t: ^testing.T) {
 test_issue_7763__Aeq :: proc(t: ^testing.T) {
 	input := "A="
 	decoded, err := base64.decode(input)
-	testing.expect(t, err == nil)
+	testing.expect_value(t, err, .Malformed_Quantum)
 	testing.expect(t, decoded == nil)
 }
 
@@ -111,7 +111,7 @@ test_issue_7763__strict_eq :: proc(t: ^testing.T) {
 test_issue_7763__strict_Aeq :: proc(t: ^testing.T) {
 	input := "A="
 	decoded, err := base64.decode(input, options = { .Strict })
-	testing.expect_value(t, err, .Invalid_Padding)
+	testing.expect_value(t, err, .Malformed_Quantum)
 	testing.expect(t, decoded == nil)
 }