Out-Of-Boundary Read when parses base64 data

[geekwish]

static int base64decode_block(unsigned char *target, const char *data, size_t data_size)
{
    int w1,w2,w3,w4;
    int i;
    size_t n;

    if (!data || (data_size <= 0)) {
        return 0;
    }   

    n = 0;
    i = 0;
    while (n < data_size-3) {
        w1 = base64_table[(int)data[n]];  // what if data[n] == -1 ?  type of data is signed char..
        w2 = base64_table[(int)data[n+1]];
        w3 = base64_table[(int)data[n+2]];
        w4 = base64_table[(int)data[n+3]];

        if (w2 >= 0) {
            target[i++] = (char)((w1*4 + (w2 >> 4)) & 255);
        }   
        if (w3 >= 0) {
            target[i++] = (char)((w2*16 + (w3 >> 2)) & 255);
        }   
        if (w4 >= 0) {
            target[i++] = (char)((w3*64 + w4) & 255);
        }   
        n+=4;
    }   
    return i;
}

And if it parse

<data trn="1.0" 0//EN"
    "http.DTDs/ProWerwwk.arsion="1.0">></data>

unsigned char *base64decode(const char *buf, size_t *size)
{
    if (!buf || !size) return NULL;
    size_t len = (*size > 0) ? *size : strlen(buf);
    if (len <= 0) return NULL;
    unsigned char *outbuf = (unsigned char*)malloc((len/4)*3+3);
    const unsigned char *ptr = buf;
    int p = 0;
    size_t l = 0;

    do {
        ptr += strspn(ptr, "\r\n\t ");
        if (*ptr == '\0' || ptr >= buf+len) {
            break;
        }   
        l = strcspn(ptr, "\r\n\t "); // out of boundary .......................
        if (l > 3 && ptr+l <= buf+len) {
            p+=base64decode_block(outbuf+p, ptr, l); 
            ptr += l;
        } else {
            break;
        }   
    } while (1);

    outbuf[p] = 0;
    *size = p;
    return outbuf;
}

one possible patch

diff --git a/src/base64.c b/src/base64.c
index 7870a79..7d6f8fd 100644
--- a/src/base64.c
+++ b/src/base64.c
@@ -71,7 +71,7 @@ size_t base64encode(char *outbuf, const unsigned char *buf, size_t size)
        return m;
 }
 
-static int base64decode_block(unsigned char *target, const char *data, size_t data_size)
+static int base64decode_block(unsigned char *target, const unsigned char *data, size_t data_size)
 {
        int w1,w2,w3,w4;
        int i;
@@ -106,19 +106,19 @@ static int base64decode_block(unsigned char *target, const char *data, size_t da
 unsigned char *base64decode(const char *buf, size_t *size)
 {
        if (!buf || !size) return NULL;
-       size_t len = (*size > 0) ? *size : strlen(buf);
+       size_t len = *size;
        if (len <= 0) return NULL;
        unsigned char *outbuf = (unsigned char*)malloc((len/4)*3+3);
-       const char *ptr = buf;
+       const unsigned char *ptr = buf;
        int p = 0;
        size_t l = 0;
 
        do {
-               ptr += strspn(ptr, "\r\n\t ");
+               ptr += strspn((const char *)ptr, "\r\n\t ");
                if (*ptr == '\0' || ptr >= buf+len) {
                        break;
                }
-               l = strcspn(ptr, "\r\n\t ");
+               l = strcspn((const char *)ptr, "\r\n\t ");
                if (l > 3 && ptr+l <= buf+len) {
                        p+=base64decode_block(outbuf+p, ptr, l);
                        ptr += l;

[nikias]

Hi, thanks for pointing this out. I noticed some other issues in the parser, like split data, so I reworked the parsing: 3a55ddd
This also fixes the OOB read and will parse your test case 'correctly' into no data output since it is not properly base64 encoded.

[carnil]

Hi

This has been assigned CVE-2017-5209.

[epozuelo]

Hi,

I created asdf.plist as @geekwish said:

emilio@tatooine$ cat asdf.plist
<data trn="1.0" 0//EN"
"http.DTDs/ProWerwwk.arsion="1.0">>

Now if I parse it with plist 1.12, I get:

emilio@tatooine$ /opt/libplist/bin/plistutil -i asdf.plist
Entity: line 1: parser error : error parsing attribute name
<data trn="1.0" 0//EN"
^
Entity: line 1: parser error : attributes construct error
<data trn="1.0" 0//EN"
^
Entity: line 1: parser error : Couldn't find end of Start Tag data line 1
<data trn="1.0" 0//EN"
^
Entity: line 1: parser error : Extra content at the end of the document
<data trn="1.0" 0//EN"
^
ERROR

With git commit 3a55ddd, with bbd3379 or with 6a44dfb I get instead:

emilio@tatooine$ /opt/libplist/bin/plistutil -i asdf.plist
bplist00@������
emilio@tatooine$

(with some unicode garbage after the "@")

Is that expected? That doesn't look right to me

[epozuelo]

Ping? I'd like to issue a security update downstream with a fix for this issue, but since I'm unable to reproduce it, I can't be sure if the fix is working on the backport.

Perhaps I have a bad test case? Can you add the correct one as an attachment?

Thanks

[nikias]

@epozuelo I am not sure what you are doing there, but if I try to convert the file

$ cat asdf.plist
<data trn="1.0" 0//EN"
"http.DTDs/ProWerwwk.arsion="1.0">>

(without enclosing <plist></plist> tags)
the output since commit 3a55ddd is the following:

$ PLIST_XML_DEBUG=1 plistutil -i asdf.plist
libplist[xmlparser] ERROR: EOF while looking for closing tag
libplist[xmlparser] ERROR: Could not parse text content for 'data' node
ERROR: Failed to convert input file.

so the aforementioned issue is already fixed and the parsing fails. No idea why you actually seem to get binary plist output though.