owasp-modsecurity / owasp-modsecurity/ModSecurity

Incorrect utf8toUnicode transformation for 00xx

Open
#3,129 3 comments 0 reactions 0 assignees View on GitHub

Nobody has claimed this yet.

2.x
Dominant language
C++
Stars
9.8k
Forks
1.8k
Avg merge
2h 46m
Merged PRs (30d)
1

Description

In utf8_unicode_inplace_ex(), we have the following code:

c = *utf;
/* If first byte begins with binary 0 it is single byte encoding */
if ((c & 0x80) == 0) {
    /* single byte unicode (7 bit ASCII equivilent) has no validation */
    count++;
    if (count <= len) {
        if (c == 0) *data = x2c(&c);
        else *data++ = c;
    }
}

The code if (c == 0) *data = x2c(&c); is wrong.
utf the input string.
If input is "x", we copy the character => correct.
If input is "\x00\xA0", we call the x2c function which expects an hexadecimal string as input (like "20" for a space).
Furthermore, the output buffer (data) is not increased, so the character is overwritten on the next loop.
The correct code is

if (c == 0) {
    sprintf(data, "%%u%04x", utf[1]);
    count += 4;
    data += 6;
    i++;
    *changed = 1;
}

Remarks:

  1. Using "%04x" replaces a lot of useless code used in other parts of the code
  2. I didn't add a check if (count <= len) after count += 4; because it cannot overflow if we fix the code: len = input_len * 6 + 1; instead of len = input_len * 4 + 1; (4 bytes -> %u1234). The check (and the variable count) could thus be removed everywhere
  3. After filling data, we have several checks (/* invalid UTF-8 character number range (RFC 3629) /, / check for overlong */, ...) with this code count++; *data++ = c; What's the point? This copies the character after the ones we already copied. This looks wrong to me. Any reason to keep this code?

Contributor guide

No contributing guide indexed for this repository

First steps

  1. Read the whole issue, then the project's contributing guide.
  2. Comment on the issue to say you are picking it up — it saves two people doing the same work.
  3. Fork the repository and make your change on a branch.
  4. Open a pull request that references the issue number.

Research direction

Start in utf8_unicode_inplace_ex() and inspect the 00xx branch around x2c, the len/input_len calculation, and the later RFC 3629 validation checks. Trace the buffer and count updates for inputs such as "x" and "\x00\xA0"; done means the transformation preserves the intended characters without overwriting output or introducing duplicate copies.

Written by the indexing model from the issue text.

Assessment

Tech stack
cpp
Domain
security
Issue type
Bug
Difficulty
3/5
Estimated time
1-2 days
Activity status
Stale
Clarity
Mostly clear
Newbie friendliness
35/100

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.