[http] sanitise URL options before use them by linev · Pull Request #22186 · root-project/root

linev · 2026-05-08T09:16:28Z

URL syntax allow to provide different special symbols using %12%20 symbols.
Also some situations un-escaped quotes can make a problems.
Therefore cleanup arguments from URL string adding C escape symbols.

Use it in exe.json and cmd.json - main place where arguments which are coming via URL
are formatted for ProcessLine.

Add sophisticated check in cmd.json processing. One need to check quotes in argument and
in command definition around argument. If none exists - only simple numeric values can be injected
without quotes. Avoid double quotes one both side of argument placeholder.

Provide special flag to enable processing of post data in the exe.json, off by default.

Provide testing for the main requests processing in the sniffer.
This emulates response of THttpServer with same kind of http requests.

github-actions · 2026-05-08T11:43:09Z

Test Results

22 files 22 suites 3d 10h 33m 7s ⏱️
3 856 tests 3 856 ✅ 0 💤 0 ❌
77 054 runs 77 054 ✅ 0 💤 0 ❌

Results for commit ab58efa.

♻️ This comment has been updated with latest results.

silverweed

In general I don't think we can really trust all this code to produce properly "sanitized" input. Sanitizers are notoriously hard to get right and I see many suspicious places in the Sniffer code.

So if these checks are in place as a measure against accidental errors, they may be useful, but I wouldn't consider them a measure against actual attacks.

Regardless, I put a couple of more specific comments.

Also, we definitely need tests for this.

silverweed · 2026-05-08T13:56:57Z

+   if (!value || !*value)
+      return "";

   TString res = value;


How long can value be? Since we're trying to sanitize the input, shouldn't we also put a reasonable max length?

There is no such limit and I cannot make here any assumption.
civetweb cuts URL length by 16K.

The backend is not guaranteed to be civetweb in principle, is it?
We should put an artificial limit here so we don't have to worry about huge allocations/reallocations/copies/iterations and we could in principle even preallocate the output string once and do a single pass over the string to validate it.

In my mind such checks are not part of this PR.
They can be done in TUrl class which used to parse all url arguments

I find it fragile to trust the assumption that all strings getting here come from some other place that already partially sanitized them, especially considering it's not even an explicit thing happening at the calling site.

That said, you can at least spare the strlen(value) == 0 and simply check res.Length() afterwards, to save one string scan.

silverweed · 2026-05-08T13:59:52Z

+         clean.Append('\\');
+         clean.Append(c);
+      } else if (c > 31)
+         // ignore all special symbols


I think c == 127 is a "special" symbol (DEL)

Probably I just will use !std::iscntrl(c)

I'm not sure what this excludes exactly (does it exclude all ASCII non-printable characters? Does it depend on the locale? The man page makes it look like the case), so I was actually more comfortable with the explicit check.

I do not like to exclude all non-latin symbols here - as long as they supported on all other places.

Main intention of this PR - prevent creation of string literals which can be mis-interpreted by cling.

You are basically already excluding non-latin symbols by doing char-based string processing, so it's really about excluding special ASCII control characters that we don't expect to ever see...

To be more precise: if Unicode support is desired then we should make sure it works, as it's not really obvious at all that we are handling it correctly throughout the entire pipeline from start to finish (and I don't think we should bother)

In DecodeUrlOptionValue test I add German letters.

silverweed · 2026-05-08T14:09:49Z

+         if (std::isalnum(c) || std::strchr(".:+-", c))
+            sanitized.Append(c);
+      } else if ((c == '\"') || (c == '\\')) {
+         // escape quotes inside string


This is quite bizarre if I understand correctly: if an argument is "ab"cdef" we will parse it as the string "ab\"cdef"?

It will be delivered to C++ as

new TMethodCall(obj_cl, method_name, "ab\"cdef" );

Shouldn't we rather require all internal quotes to be already escaped in the argument? If the incoming argument is supposed to be a valid string literal it would not be valid if it contains unescaped quotes, which may indicate some other kind of error (on the user side or otherwise)

Shouldn't we rather require all internal quotes to be already escaped in the argument?

On each layer escape character can disappear.
Quotes are very special - because when they goes into C++ interpreter they can break argument list and start new command. By such transformation we want guarantee that in any case interpreter will take provided string as single argument. Actually exactly such exploit is shown in [THttpServer-4] issue.
So attempt here to produce string which not closed in between.

On each layer escape character can disappear.

That sounds like a bug from the previous layers...
Regardless, it would be nice to have some tests on this and comments to make it clear why we're doing this and exactly what form of strings do we accept from the user side

There are many differences how quotes escaped in URL and in C++.
Main problem - string is used again in C++ script. So one need to provide/create escape characters.

Even so, please add unit tests to verify/document the expected behavior of this code before merging.

I add testing of 4 different combinations how quotes can appear in command definition and in command invocation.

Thanks.

I think we should also add unit tests for the DecodeUrlOptionValue function, passing all kinds of "antagonistic" expressions that we expect to be sanitized by it.

I add to last commit extra tests for DecodeUrlOptionValue method

Remove any special symbols Add escape characters for quote and escape itself Try to avoid manipulation of arguments for method execution

Avoid special characters as draw arguments

Always use DecodeUrlOptionValue method when processing URL arguments or URL string. Internally method provides escape symbols for quotes and backslash. If expecting numeric value - remove all symbols keeping alphanumeric, '.', '+', '-' and ':'

It allows to deserialize post data as ROOT object when processing exe.json request. While this can leads to arbitrary code loading and injection, disable this feature by default. Can be enabled back with: ``` serv->SetAllowPostObject(kTRUE); ```

While here arbitrary string injected into ProcessLine, ensure that only numeric argument is not quoted. All other arguments kinds will be quoted and prevent execution of potentially dangerous code

silverweed · 2026-05-19T11:20:09Z

+   EXPECT_EQ(test.Decode("\"\"\"", kFALSE), "\"\\\"\"");
+
+   // keep quotes and keep german letters - remove new line
+   EXPECT_EQ(test.Decode("\"Gänse\nfüßchen\"", kFALSE), "\"Gänsefüßchen\"");


Could you please also add strings containing "operator-like" things, like someFunc("someArg"), someArray[3], and verify they get stripped?

I add one - but only quotes are escaped, nothing else is changed.

Only later such special string will be surrounded by normal quotes or discarded while it does not match to numeric value.

Verify execution of several supported requests which can be handled by http server. Testing: - root.json - root.xml - exe.json - exe.json with POST data - item.json - cmd.json - multi.json Also verify basic functionality of TRootSniffer::DecodeUrlOptionValue method

linev requested review from dpiparo and jblomer May 8, 2026 09:16

linev self-assigned this May 8, 2026

linev added affects:master in:WebGui labels May 8, 2026

dpiparo changed the title ~~[http] sanity URL options before use them~~ [http] sanitise URL options before use them May 8, 2026

jblomer approved these changes May 8, 2026

View reviewed changes

Comment thread net/httpsniff/src/TRootSnifferFull.cxx Outdated

silverweed reviewed May 8, 2026

View reviewed changes

linev force-pushed the http_sanity_arg branch from 3668ee2 to 83b7d75 Compare May 8, 2026 14:37

linev added 2 commits May 8, 2026 16:43

[http] cleanup URL option before usage

d4c1aa2

Remove any special symbols Add escape characters for quote and escape itself Try to avoid manipulation of arguments for method execution

[http] cleanup draw option in image production

0433163

Avoid special characters as draw arguments

linev force-pushed the http_sanity_arg branch from 83b7d75 to 2b46cab Compare May 8, 2026 14:44

linev added 2 commits May 18, 2026 13:11

[http] introduce fAllowPostObject flag

fa53f87

It allows to deserialize post data as ROOT object when processing exe.json request. While this can leads to arbitrary code loading and injection, disable this feature by default. Can be enabled back with: ``` serv->SetAllowPostObject(kTRUE); ```

linev force-pushed the http_sanity_arg branch from cc20b42 to fa53f87 Compare May 18, 2026 11:18

linev mentioned this pull request May 18, 2026

[http] introduce TRootSniffer::fAllowPostObject flag #22192

Closed

[http] analyze arguments of cmd.json

772aa0f

While here arbitrary string injected into ProcessLine, ensure that only numeric argument is not quoted. All other arguments kinds will be quoted and prevent execution of potentially dangerous code

linev requested a review from bellenot as a code owner May 18, 2026 15:55

linev requested a review from silverweed May 18, 2026 15:59

linev force-pushed the http_sanity_arg branch 3 times, most recently from 69ba55a to c4d0d1f Compare May 19, 2026 11:01

silverweed reviewed May 19, 2026

View reviewed changes

linev force-pushed the http_sanity_arg branch from c4d0d1f to ab58efa Compare May 19, 2026 11:24

Conversation

linev commented May 8, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

github-actions Bot commented May 8, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Test Results

Uh oh!

Uh oh!

silverweed left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

silverweed May 19, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

linev commented May 8, 2026 •

edited

Loading

github-actions Bot commented May 8, 2026 •

edited

Loading

silverweed left a comment •

edited

Loading

silverweed May 19, 2026 •

edited

Loading