Skip to content

[http] sanitise URL options before use them #22186

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
linev wants to merge 4 commits into
base: master
Choose a base branch
from
Open

[http] sanitise URL options before use them #22186

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
d4c1aa2
[http] cleanup URL option before usage
linev May 8, 2026
0433163
[http] cleanup draw option in image production
linev May 8, 2026
2b46cab
[http] strictly check arguments in ProduceExe
linev May 8, 2026
cc20b42
[http] sanitize class name of post object
linev May 8, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
37 changes: 32 additions & 5 deletions net/http/src/TRootSniffer.cxx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1289,8 +1289,8 @@ Bool_t TRootSniffer::ProduceXml(const std::string &/* path */, const std::string

TString TRootSniffer::DecodeUrlOptionValue(const char *value, Bool_t remove_quotes)
{
if (!value || (strlen(value) == 0))
return TString();
if (!value || !*value)
return "";

TString res = value;
Copy link
Copy Markdown
Contributor

@silverweed silverweed May 8, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Copy Markdown
Member Author

@linev linev May 8, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Copy Markdown
Contributor

@silverweed silverweed May 8, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.


Expand All @@ -1303,13 +1303,40 @@ TString TRootSniffer::DecodeUrlOptionValue(const char *value, Bool_t remove_quot
res.ReplaceAll("%5D", "]");
res.ReplaceAll("%3D", "=");
Copy link
Copy Markdown
Contributor

@silverweed silverweed May 8, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This goes over the string 8 times, whereas we could do all of this in 1 pass...(this is in addition to the TString constructor, which also does a full scan of the string to determine the length).
Depending on the max length we impose for the string and whether denial of service is a concern, we might want to fix this.

Copy link
Copy Markdown
Member Author

@linev linev May 8, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Deny of access or deny of service is already problematic with pure civetweb.
It is not designed to protect from such attacks - it should be done in front of it.

Copy link
Copy Markdown
Contributor

@silverweed silverweed May 8, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's still a very suboptimal way to do this validation that scales superlinearly with the arguments' length. But if this is not a concern for this class, fine for me.


if (remove_quotes && (res.Length() > 1) && ((res[0] == '\'') || (res[0] == '\"')) &&
(res[0] == res[res.Length() - 1])) {
Char_t quote = 0;

if ((res.Length() > 1) && ((res[0] == '\'') || (res[0] == '\"')) && (res[0] == res[res.Length() - 1]))
quote = res[0];

// first remove quotes
if (quote) {
res.Remove(res.Length() - 1);
res.Remove(0, 1);
}

return res;
// we expect normal content here, no special symbols, no unescaped quotes
TString clean;
for (Ssiz_t i = 0; i < res.Length(); ++i) {
char c = res[i];
if (c == '"' || c == '\\') {
// escape quotes and slahes
clean.Append('\\');
clean.Append(c);
} else if (!std::iscntrl(c))
// ignore all special symbols
Copy link
Copy Markdown
Contributor

@silverweed silverweed May 8, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Copy Markdown
Member Author

@linev linev May 8, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Copy Markdown
Contributor

@silverweed silverweed May 8, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

clean.Append(c);
}

if (quote && !remove_quotes) {
// return string with quotes - when desired
res = "";
res.Append(quote);
res.Append(clean);
res.Append(quote);
return res;
}

return clean;
}

////////////////////////////////////////////////////////////////////////////////
Expand Down
3 changes: 3 additions & 0 deletions net/httpsniff/inc/TRootSnifferFull.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@
#include <string>

class TMemFile;
class TMethodArg;

class TRootSnifferFull : public TRootSniffer {
protected:
Expand All @@ -30,6 +31,8 @@ class TRootSnifferFull : public TRootSniffer {

void CreateMemFile();

static TString SanitizeArgument(Bool_t isstr, const char *value, Bool_t onlyquotes = kFALSE);

Bool_t CanDrawClass(TClass *cl) override { return IsDrawableClass(cl); }

Bool_t HasStreamerInfo() const override { return kTRUE; }
Expand Down
66 changes: 52 additions & 14 deletions net/httpsniff/src/TRootSnifferFull.cxx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -416,7 +416,7 @@ Bool_t TRootSnifferFull::ProduceImage(Int_t kind, const std::string &path, const
if (gDebug > 1)
Info("TRootSniffer", "Crate IMAGE from object %s", obj->GetName());

Int_t width(300), height(200);
Int_t width = 300, height = 200;
TString drawopt;

if (!options.empty()) {
Expand All @@ -429,9 +429,7 @@ Bool_t TRootSnifferFull::ProduceImage(Int_t kind, const std::string &path, const
Int_t h = url.GetIntValueFromOptions("h");
if (h > 10)
height = h;
const char *opt = url.GetValueFromOptions("opt");
if (opt)
drawopt = opt;
drawopt = DecodeUrlOptionValue(url.GetValueFromOptions("opt"), kTRUE);
}

Bool_t isbatch = gROOT->IsBatch();
Expand Down Expand Up @@ -513,6 +511,42 @@ Bool_t TRootSnifferFull::ProduceXml(const std::string &path, const std::string &
return !res.empty();
}

////////////////////////////////////////////////////////////////////////////////
/// Verify argument value
/// Remove special symbols, escape quotes
/// If not string - only digits, literals, "+", "-", "." are allowed

TString TRootSnifferFull::SanitizeArgument(Bool_t isstr, const char *value, Bool_t onlyquotes)
{
Int_t beg = 0, end = strlen(value);
Copy link
Copy Markdown
Contributor

@silverweed silverweed May 8, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is value guaranteed to have a reasonable max length here? Otherwise strlen is potentially dangerous.

Copy link
Copy Markdown
Member Author

@linev linev May 8, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Value created from URL and will not be longer than 16K

Copy link
Copy Markdown
Contributor

@silverweed silverweed May 8, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same comment as above: we should put our own limit as well

if (end > 0 && isstr && value[beg] == '\"')
beg++;
if (end > 0 && isstr && value[end-1] == '\"')
end--;
TString sanitized;
if (isstr)
sanitized.Append("\""); // string start with quotes
for(Int_t n = beg; n < end; ++n) {
Char_t c = value[n];
if (!isstr && !onlyquotes) {
// for numeric types make very strict check
if (std::isalnum(c) || std::strchr(".:+-", c))
sanitized.Append(c);
Copy link
Copy Markdown
Contributor

@silverweed silverweed May 8, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This will not necessarily produce a number, so I'm not sure what's the goal of this sanitization (what if the string is -4+3.2-245+++++67?)

Copy link
Copy Markdown
Member Author

@linev linev May 8, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Goal to avoid symbols sequence which can be interpreted as C/C++ code invocation like gSystem->DoSomething()

Copy link
Copy Markdown
Contributor

@silverweed silverweed May 8, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You're checking for isalnum though: did you mean to check for isdigit? Otherwise I wouldn't rule out "interesting" code invocations going through custom arithmetic operators...

Copy link
Copy Markdown
Contributor

@silverweed silverweed May 8, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Even if every non-operator is a digit this will still cause N>=1 expressions to be evaluated, which I guess is safe, but it will still go through the interpreter to be executed before being passed to TMethodCall I think.

} else if ((c == '\"') || (c == '\\')) {
// escape quotes inside string
Copy link
Copy Markdown
Contributor

@silverweed silverweed May 8, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Copy Markdown
Member Author

@linev linev May 8, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It will be delivered to C++ as

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

Copy link
Copy Markdown
Contributor

@silverweed silverweed May 8, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)

Copy link
Copy Markdown
Member Author

@linev linev May 8, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Copy Markdown
Contributor

@silverweed silverweed May 8, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Copy Markdown
Member Author

@linev linev May 8, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

sanitized.Append('\\');
sanitized.Append(c);
} else if (!std::iscntrl(c)) {
// exclude special chars
sanitized.Append(c);
}
}
if (isstr)
sanitized.Append('\"'); // string end with quotes
return sanitized;
}


////////////////////////////////////////////////////////////////////////////////
/// Execute command for specified object
///
Expand Down Expand Up @@ -638,6 +672,10 @@ Bool_t TRootSnifferFull::ProduceExe(const std::string &path, const std::string &
val = sval.Data();
}

// only value provided from outside should be sanitized
// default argument value or internal pointer will not
Bool_t sanitize = kFALSE;
Copy link
Copy Markdown
Contributor

@jblomer jblomer May 8, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it would be useful to have a comment here explaining when/why we do not sanitize.

Copy link
Copy Markdown
Member Author

@linev linev May 8, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When processing POST data special value created directly in the code - including pointer casting.
Such values should not be touched.
Value also not sanitized when default value from method arguments list is extracted.
In other cases value will be checked

Copy link
Copy Markdown
Contributor

@jblomer jblomer May 8, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks! This as a code comment would be helpful.


if ((val != nullptr) && (strcmp(val, "_this_") == 0)) {
// special case - object itself is used as argument
sval.Form("(%s*)0x%zx", obj_cl->GetName(), (size_t)obj_ptr);
Expand Down Expand Up @@ -667,7 +705,7 @@ Bool_t TRootSnifferFull::ProduceExe(const std::string &path, const std::string &
}
val = sval.Data();
} else if ((strcmp(val, "_post_object_") == 0) && url.HasOption("_post_class_")) {
TString clname = url.GetValueFromOptions("_post_class_");
TString clname = DecodeUrlOptionValue(url.GetValueFromOptions("_post_class_"), kTRUE);
TClass *arg_cl = gROOT->GetClass(clname, kTRUE, kTRUE);
if ((arg_cl != nullptr) && (arg_cl->GetBaseClassOffset(TObject::Class()) == 0) && (post_obj == nullptr)) {
post_obj = (TObject *)arg_cl->New();
Expand All @@ -692,7 +730,11 @@ Bool_t TRootSnifferFull::ProduceExe(const std::string &path, const std::string &
} else if (strcmp(val, "_post_length_") == 0) {
sval.Form("%ld", (long)fCurrentArg->GetPostDataLength());
val = sval.Data();
} else {
sanitize = kTRUE;
}
} else {
sanitize = val != nullptr;
}

if (!val)
Expand All @@ -708,15 +750,11 @@ Bool_t TRootSnifferFull::ProduceExe(const std::string &path, const std::string &
if (call_args.Length() > 0)
call_args += ", ";

if ((strcmp(arg->GetFullTypeName(), "const char*") == 0) || (strcmp(arg->GetFullTypeName(), "Option_t*") == 0)) {
int len = strlen(val);
if ((strlen(val) < 2) || (*val != '\"') || (val[len - 1] != '\"'))
call_args.Append(TString::Format("\"%s\"", val));
else
call_args.Append(val);
} else {
call_args.Append(val);
}
Bool_t isstr = (strcmp(arg->GetFullTypeName(), "const char*") == 0) ||
(strcmp(arg->GetFullTypeName(), "Option_t*") == 0) ||
(strcmp(arg->GetFullTypeName(), "string") == 0);

call_args.Append(SanitizeArgument(isstr, val, !sanitize));
}

TMethodCall *call = nullptr;
Expand Down
Loading