nginx / nginx/njs

Modules: CR/LF in njs-produced nginx variables

Open
#1,127 0 comments 0 reactions 0 assignees View on GitHub

Nobody has claimed this yet.

Dominant language
C
Stars
1.6k
Forks
239
Avg merge
2d 9h
Merged PRs (30d)
10

Description

#1111 added header-value validation to r.headersOut and to redirect targets passed through r.return().

In the review for #1111, the remaining raw conversion in ngx_http_js_request_variables() was noted as:

not a header path -- correctly left alone.

There is a consequence of that distinction when an njs-produced variable is later consumed by a directive that serializes it into an HTTP header.

The shortest case uses js_set:

function v(r) {
    return r.args.v;
}

export default { v };
js_import test from test.js;
js_set $fromjs test.v;

location /set {
    add_header X-Test $fromjs;
    return 200 "ok\n";
}

Requesting:

/set?v=a%0d%0aInjected:%201

produces:

X-Test: a
Injected: 1

The value is already decoded when the handler sees it. Reporting the character codes of r.args.v for the same request gives:

97,13,10,...

so CR and LF are present as actual characters rather than as the encoded %0d%0a text.

The equivalent direct header store is rejected:

r.headersOut['X-Test'] = r.args.v;
// TypeError: invalid header value

while both of these preserve the CR/LF:

// js_set
return r.args.v;

// r.variables
r.variables.injected = r.args.v;

The r.variables case can also reach an upstream request header:

function up(r) {
    r.variables.injected = r.args.v;
    r.internalRedirect('/proxied');
}

export default { up };
js_import test from test.js;
js_var $injected;

location /up {
    js_content test.up;
}

location /proxied {
    internal;
    proxy_set_header X-Fwd $injected;
    proxy_pass http://127.0.0.1:9002;
}

Requesting:

/up?v=a%0d%0aX-Smuggled:%20yes

makes nginx send upstream:

GET /proxied HTTP/1.0
X-Fwd: a
X-Smuggled: yes
Host: 127.0.0.1:9002
Connection: close

This behavior is not unique to njs variables.

For example, $arg_v is not decoded:

add_header X-Test $arg_v;

and therefore produces:

X-Test: a%0d%0aInjected:%201

On the other hand, nginx variables can themselves contain decoded data. For example, $uri can produce the same header splitting behavior when used with add_header, with no njs involved.

So I am not assuming that generic nginx variables are required to satisfy HTTP header-value syntax.

The njs-specific case is that request data such as r.args.v is exposed decoded, and js_set or r.variables can carry that value into a generic nginx variable which is then consumed by a header-producing directive.

Given the distinction made in #1111, is preserving CR/LF in njs-produced nginx variables considered intentional even when those variables are later consumed by directives such as add_header or proxy_set_header?

As a sanity check, I locally tested rejecting only CR and LF at the points where an njs value becomes an nginx variable, including both js_set and r.variables.

With that change:

  • the js_set example no longer emits X-Test;
  • assigning the same value through r.variables throws TypeError: invalid variable value;
  • the upstream-header case is no longer possible;
  • ordinary values remain unchanged, including plain, UTF-8 such as café, and values containing a tab.

I limited the test to CR and LF rather than applying the full ngx_js_check_header_value() rules, since nginx variables are generic and may be consumed outside HTTP headers.

Tested with njs 1.0.1 built into nginx 1.28.0, using both the default engine and js_engine qjs; the behavior is identical with both.

I also ran the 104 nginx/t js and stream_js test files (1003 assertions) against the changed and unchanged builds. The pass/fail sets were identical. Twelve files fail in both builds in my ASan -j4 environment, so there was no regression delta from this change.

Contributor guide

Open the contributing guide

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 by reading ngx_http_js_request_variables() and the paths handling js_set and r.variables. Reproduce the CR/LF cases with add_header and proxy_set_header, then compare behavior for plain, UTF-8, and tab-containing values. Run the nginx/t JavaScript and stream_js test suites; done means the intended variable behavior is resolved and covered without changing unrelated values.

Written by the indexing model from the issue text.

Assessment

Tech stack
c, javascript, nginx
Domain
backend, security
Issue type
Bug
Difficulty
4/5
Estimated time
3-5 days
Activity status
Active
Clarity
Mostly clear
Newbie friendliness
45/100

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.