Project

General

Profile

Bug #2848

buffer_append_base64_decode() broken on compilers where char is assumed unsigned

Added by codehero 10 months ago. Updated 10 months ago.

Status:
Fixed
Priority:
Normal
Assignee:
-
Category:
mod_auth
Target version:
Start date:
2017-12-20
Due date:
% Done:

100%

Estimated time:
Missing in 1.5.x:

Description

In my application, the username/password combination "admin" "admin" did not work on lighttpd 1.4.48 using basic auth.
Yes, admin/admin was borken.
After many hours of digging through lighttpd code, I determined that the following static const variables in base64.c:

static const char base64_standard_reverse_table[]

static const char base64_url_reverse_table[]

should actually be explicit signed characters, as char.

The attached patch resolves the issue.

base64_fix_decode.patch (339 Bytes) base64_fix_decode.patch patch to resolve issue codehero, 2017-12-20 07:18

Related issues

Has duplicate Bug #2851: mod_auth fails its test suite on PowerPC (big-endian?)Duplicate2017-12-31

Actions
Has duplicate Bug #2873: Basic Auth method not workingDuplicate2018-03-02

Actions

Associated revisions

Revision d4083eff (diff)
Added by gstrauss 10 months ago

[core] fix base64 decode when char is unsigned (fixes #2848)

thx, codehero

x-ref:
"buffer_append_base64_decode() broken on compilers where char is assumed unsigned"
https://redmine.lighttpd.net/issues/2848

History

#1

Updated by stbuehler 10 months ago

Please use unified diffs (diff -u) for patches; and could you also add a test case in test_base64.c which fails on your platform?

#2

Updated by gstrauss 10 months ago

What operating system are you on and which compiler are you using?

Your patch would result in additional signed/unsigned warnings/errors from other compilers.

A more portable fix might be to modify -1 -2 -3 values in the tables to be out of range for base64, and check for those values for errors, rather than -1 -2 -3.

Using unsigned char would also be better than explicit signed char, but would also require additional casting.

For your system, which must be treating char as unsigned char, this might be the simplest fix:

--- a/src/base64.c
+++ b/src/base64.c
@@ -72,7 +72,7 @@ unsigned char* buffer_append_base64_decode(buffer *out, const char* in, size_t i
                        break;
                } else if (-2 == ch) {
                        continue; /* skip character */
-               } else if (ch < 0) {
+               } else if (-1 == ch) {
                        return NULL; /* invalid character, abort */
                }

You should check your compiler options and consider compiling lighttpd with explicit compiler switch to treat 'char' as 'signed char'

#3

Updated by codehero 10 months ago

For your system, which must be treating char as unsigned char, this might be the simplest fix:
[...]

You should check your compiler options and consider compiling lighttpd with explicit compiler switch to treat 'char' as 'signed char'

Is this expectation for lighttpd or just for this function??
This is the most important point to me.
If for all of lighttpd, then your build system must address this is.

And to be honest, I don't have the time to further pursue a solution for the issue; I have to move on.
I am trying to get something delivered and mod_authn_ldap is leaking memory like crazy.

#4

Updated by stbuehler 10 months ago

codehero wrote:

For your system, which must be treating char as unsigned char, this might be the simplest fix:
[...]

You should check your compiler options and consider compiling lighttpd with explicit compiler switch to treat 'char' as 'signed char'

Is this expectation for lighttpd or just for this function??
This is the most important point to me.
If for all of lighttpd, then your build system must address this is.

I agree. And imho lighttpd should handle char == unsigned char.

And to be honest, I don't have the time to further pursue a solution for the issue; I have to move on.
I am trying to get something delivered and mod_authn_ldap is leaking memory like crazy.

"leaking memory" - you got any valgrind traces for that?

#5

Updated by codehero 10 months ago

I agree. And imho lighttpd should handle char == unsigned char.

Sounds good to me; hope you guys can decide on it.

And to be honest, I don't have the time to further pursue a solution for the issue; I have to move on.
I am trying to get something delivered and mod_authn_ldap is leaking memory like crazy.

"leaking memory" - you got any valgrind traces for that?

I opened up bug https://redmine.lighttpd.net/issues/2849

#6

Updated by gstrauss 10 months ago

When building with the following patch on x86_64 and compiling with -funsigned-char, make check passes, but without this patch, make check fails when building with -funsigned-char. While lighttpd could always use more test cases, nothing else immediately jumps out as depending on 'char' to be 'signed char'.

diff --git a/src/base64.c b/src/base64.c
index f39dbaa2..3034181a 100644
--- a/src/base64.c
+++ b/src/base64.c
@@ -11,7 +11,7 @@

 /* BASE64_STANDARD: "A-Z a-z 0-9 + /" maps to 0-63, pad with "=" */
 static const char base64_standard_table[] = "ABCDEFGHIJKLMNOPQRSTUVWXYZabcdefghijklmnopqrstuvwxyz0123456789+/=";
-static const char base64_standard_reverse_table[] = {
+static const signed char base64_standard_reverse_table[] = {
 /*      0   1   2   3   4   5   6   7   8   9   A   B   C   D   E   F */
        -1, -2, -2, -2, -2, -2, -2, -2, -2, -2, -2, -2, -2, -2, -2, -2, /* 0x00 - 0x0F */
        -2, -2, -2, -2, -2, -2, -2, -2, -2, -2, -2, -2, -2, -2, -2, -2, /* 0x10 - 0x1F */
@@ -25,7 +25,7 @@ static const char base64_standard_reverse_table[] = {

 /* BASE64_URL: "A-Z a-z 0-9 - _" maps to 0-63, pad with "." */
 static const char base64_url_table[] = "ABCDEFGHIJKLMNOPQRSTUVWXYZabcdefghijklmnopqrstuvwxyz0123456789-_.";
-static const char base64_url_reverse_table[] = {
+static const signed char base64_url_reverse_table[] = {
 /*      0   1   2   3   4   5   6   7   8   9   A   B   C   D   E   F */
        -1, -2, -2, -2, -2, -2, -2, -2, -2, -2, -2, -2, -2, -2, -2, -2, /* 0x00 - 0x0F */
        -2, -2, -2, -2, -2, -2, -2, -2, -2, -2, -2, -2, -2, -2, -2, -2, /* 0x10 - 0x1F */
@@ -42,7 +42,7 @@ unsigned char* buffer_append_base64_decode(buffer *out, const char* in, size_t i
        size_t out_pos = 0; /* current output character (position) that is decoded. can contain partial result */
        unsigned int group = 0; /* how many base64 digits in the current group were decoded already. each group has up to 4 digits */
        size_t i;
-       const char *base64_reverse_table;
+       const signed char *base64_reverse_table;

        switch (charset) {
        case BASE64_STANDARD:

#7

Updated by gstrauss 10 months ago

bug introduced in lighttpd 1.4.46 in commit 7aff5046

#8

Updated by gstrauss 10 months ago

  • Status changed from Patch Pending to Fixed
  • % Done changed from 0 to 100
#9

Updated by gstrauss 10 months ago

  • Has duplicate Bug #2851: mod_auth fails its test suite on PowerPC (big-endian?) added
#10

Updated by gstrauss 8 months ago

  • Has duplicate Bug #2873: Basic Auth method not working added

Also available in: Atom