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 6 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
6 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
5b99c6e
[http] strictly check arguments in ProduceExe
linev May 8, 2026
fa53f87
[http] introduce fAllowPostObject flag
linev May 8, 2026
772aa0f
[http] analyze arguments of cmd.json
linev May 18, 2026
ab58efa
[test] add ROOT sniffer testing
linev May 18, 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
4 changes: 4 additions & 0 deletions net/http/inc/THttpServer.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -94,6 +94,10 @@ class THttpServer : public TNamed {

void SetReadOnly(Bool_t readonly = kTRUE);

Bool_t IsAllowPostObject() const;

void SetAllowPostObject(Bool_t allow_post_obj);

Bool_t IsWSOnly() const;

void SetWSOnly(Bool_t on = kTRUE);
Expand Down
7 changes: 7 additions & 0 deletions net/http/inc/TRootSniffer.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -120,6 +120,7 @@ class TRootSniffer : public TNamed {
protected:
TString fObjectsPath; ///<! default path for registered objects
Bool_t fReadOnly{kTRUE}; ///<! indicate if sniffer allowed to change ROOT structures - like read objects from file
Bool_t fAllowPostObject{kFALSE}; ///<! when true allow to deserialize objects received via POST requests
Bool_t fScanGlobalDir{kTRUE}; ///<! when enabled (default), scan gROOT for histograms, canvases, open files
std::unique_ptr<TFolder> fTopFolder; ///<! own top TFolder object, used for registering objects
THttpCallArg *fCurrentArg{nullptr}; ///<! current http arguments (if any)
Expand Down Expand Up @@ -192,6 +193,12 @@ class TRootSniffer : public TNamed {
/** Returns readonly mode */
Bool_t IsReadOnly() const { return fReadOnly; }

/** Allow to deserialize object in POST requests, default off */
void SetAllowPostObject(Bool_t allow_post_obj) { fAllowPostObject = allow_post_obj; }

/** Is allowed to deserialize object in POST requests, default off */
Bool_t IsAllowPostObject() const { return fAllowPostObject; }

void Restrict(const char *path, const char *options);

Bool_t HasRestriction(const char *item_name);
Expand Down
25 changes: 25 additions & 0 deletions net/http/src/THttpServer.cxx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -275,6 +275,31 @@ void THttpServer::SetReadOnly(Bool_t readonly)
fSniffer->SetReadOnly(readonly);
}

////////////////////////////////////////////////////////////////////////////////
/// Returns true if server accept object content in POST reequests

Bool_t THttpServer::IsAllowPostObject() const
{
return fSniffer ? fSniffer->IsAllowPostObject() : kFALSE;
}

////////////////////////////////////////////////////////////////////////////////
/// Set flag to allow receive and desereilize objects in POST requests
///
/// When object methods are executed via exe.json request,
/// one can supply object as binary/json/xml in the body of POST request
/// To allow creation of such object, one need to enable this flag
/// Use of exe.json only possible in not-readonly mode
///
/// CAUTION! This is sensitive functionality and therefore should be
/// used only when server not exposed to publicaly-accessed netowork.

void THttpServer::SetAllowPostObject(Bool_t allow_post_obj)
{
if (fSniffer)
fSniffer->SetAllowPostObject(allow_post_obj);
}

////////////////////////////////////////////////////////////////////////////////
/// returns true if only websockets are handled by the server
///
Expand Down
75 changes: 68 additions & 7 deletions net/http/src/TRootSniffer.cxx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,8 @@
#include <memory>
#include <vector>
#include <cstring>
#include <cctype>


const char *item_prop_kind = "_kind";
const char *item_prop_more = "_more";
Expand Down Expand Up @@ -1213,9 +1215,41 @@ Bool_t TRootSniffer::ExecuteCmd(const std::string &path, const std::string &opti
return kTRUE;
}

TString svalue = DecodeUrlOptionValue(argvalue, kTRUE);
argname = TString("%") + argname + TString("%");
method.ReplaceAll(argname, svalue);
auto p = method.Index(argname);
if (p == kNPOS)
continue;

method.Remove(p, argname.Length());

if ((p > 0) && (p < method.Length()) && (method.Length() > 1) && (method[p-1] == '"') && (method[p] == '"')) {
// command definition has quotes around argument
// one can insert value from URL removing quotes
method.Insert(p, DecodeUrlOptionValue(argvalue, kTRUE));
continue;
}

// extract argument without removing quotes
TString svalue = DecodeUrlOptionValue(argvalue, kFALSE);

if ((svalue.Length() > 1) && (svalue[0] == '"') && (svalue[svalue.Length() - 1] == '"')) {
// if value itself has quotes, all special symbols already escaped and one can insert it as is
method.Insert(p, svalue);
continue;
}

Bool_t is_numeric = kTRUE;
// expect decimal, hex or float values here, E/e also belong to hex
for(Size_t i = 0; is_numeric && (i < svalue.Length()); ++i)
is_numeric = std::isxdigit(svalue[i]) || std::strchr(".+-", svalue[i]);

// always quote content which not numeric
if (!is_numeric)
svalue = "\"" + svalue + "\"";
else if (svalue.IsNull())
svalue = "0";

method.Insert(p, svalue);
}
}

Expand Down Expand Up @@ -1289,8 +1323,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.

Copy link
Copy Markdown
Member Author

@linev linev May 19, 2026

Choose a reason for hiding this comment

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

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

Copy link
Copy Markdown
Contributor

@silverweed silverweed May 19, 2026

Choose a reason for hiding this comment

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

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.


Expand All @@ -1303,13 +1337,40 @@ TString TRootSniffer::DecodeUrlOptionValue(const char *value, Bool_t remove_quot
res.ReplaceAll("%5D", "]");
res.ReplaceAll("%3D", "=");
Comment thread
linev marked this conversation as resolved.

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.

Copy link
Copy Markdown
Member Author

@linev linev May 19, 2026

Choose a reason for hiding this comment

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

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.

Copy link
Copy Markdown
Contributor

@silverweed silverweed May 19, 2026 •
edited
Loading

Choose a reason for hiding this comment

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

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)

Copy link
Copy Markdown
Member Author

@linev linev May 19, 2026

Choose a reason for hiding this comment

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

In DecodeUrlOptionValue test I add German letters.

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
8 changes: 5 additions & 3 deletions net/httpsniff/CMakeLists.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,12 +1,12 @@
# Copyright (C) 1995-2019, Rene Brun and Fons Rademakers.
# Copyright (C) 1995-2026, Rene Brun and Fons Rademakers.
# All rights reserved.
#
# For the licensing terms see $ROOTSYS/LICENSE.
# For the list of contributors see $ROOTSYS/README/CREDITS.

############################################################################
# CMakeLists.txt file for building ROOT net/http package
# @author Pere Mato, CERN
# CMakeLists.txt file for building ROOT net/httpsniff package
# @author Sergey Linev, GSI
############################################################################

ROOT_STANDARD_LIBRARY_PACKAGE(RHTTPSniff
Expand All @@ -24,3 +24,5 @@ ROOT_STANDARD_LIBRARY_PACKAGE(RHTTPSniff
Tree
XMLIO
)

ROOT_ADD_TEST_SUBDIRECTORY(test)
107 changes: 63 additions & 44 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 @@ -627,47 +625,46 @@ Bool_t TRootSnifferFull::ProduceExe(const std::string &path, const std::string &
return debug != nullptr;
const char *rest_url = pos + strlen(method_name) + 7;
if (*rest_url == '&') ++rest_url;
call_args.Form("\"%s\"", rest_url);
call_args.Append("\"");
call_args.Append(DecodeUrlOptionValue(rest_url, kTRUE));
call_args.Append("\"");
break;
}

TString sval;
const char *val = url.GetValueFromOptions(arg->GetName());
if (val) {
sval = DecodeUrlOptionValue(val, kFALSE);
val = sval.Data();
}
if (val)
sval = DecodeUrlOptionValue(val, kTRUE);

Bool_t sanitize_numeric = kFALSE;

if ((val != nullptr) && (strcmp(val, "_this_") == 0)) {
if (sval == "_this_") {
// special case - object itself is used as argument
sval.Form("(%s*)0x%zx", obj_cl->GetName(), (size_t)obj_ptr);
val = sval.Data();
} else if ((val != nullptr) && (fCurrentArg != nullptr) && (fCurrentArg->GetPostData() != nullptr)) {
} else if ((fCurrentArg != nullptr) && (fCurrentArg->GetPostData() != nullptr)) {
// process several arguments which are specific for post requests
if (strcmp(val, "_post_object_xml_") == 0) {
if (fAllowPostObject && (sval == "_post_object_xml_")) {
// post data has extra 0 at the end and can be used as null-terminated string
post_obj = TBufferXML::ConvertFromXML((const char *)fCurrentArg->GetPostData());
if (!post_obj) {
if (!post_obj)
sval = "0";
} else {
else {
sval.Form("(%s*)0x%zx", post_obj->ClassName(), (size_t)post_obj);
if (url.HasOption("_destroy_post_"))
garbage.Add(post_obj);
}
val = sval.Data();
} else if (strcmp(val, "_post_object_json_") == 0) {
} else if (fAllowPostObject && (sval == "_post_object_json_")) {
// post data has extra 0 at the end and can be used as null-terminated string
post_obj = TBufferJSON::ConvertFromJSON((const char *)fCurrentArg->GetPostData());
if (!post_obj) {
if (!post_obj)
sval = "0";
} else {
else {
sval.Form("(%s*)0x%zx", post_obj->ClassName(), (size_t)post_obj);
if (url.HasOption("_destroy_post_"))
garbage.Add(post_obj);
}
val = sval.Data();
} else if ((strcmp(val, "_post_object_") == 0) && url.HasOption("_post_class_")) {
TString clname = url.GetValueFromOptions("_post_class_");
} else if (fAllowPostObject && (sval == "_post_object_") && url.HasOption("_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 @@ -684,39 +681,61 @@ Bool_t TRootSnifferFull::ProduceExe(const std::string &path, const std::string &
garbage.Add(post_obj);
}
}
sval.Form("(%s*)0x%zx", clname.Data(), (size_t)post_obj);
val = sval.Data();
} else if (strcmp(val, "_post_data_") == 0) {
if (!post_obj)
sval = "0";
else
sval.Form("(%s*)0x%zx", clname.Data(), (size_t)post_obj);
} else if (sval == "_post_data_")
sval.Form("(void*)0x%zx", (size_t)fCurrentArg->GetPostData());
val = sval.Data();
} else if (strcmp(val, "_post_length_") == 0) {
else if (sval == "_post_length_")
sval.Form("%ld", (long)fCurrentArg->GetPostDataLength());
val = sval.Data();
}
}
else
sanitize_numeric = kTRUE;
} else
sanitize_numeric = kTRUE;

if (!val)
val = arg->GetDefault();
if (sval.IsNull() && arg->GetDefault())
sval = arg->GetDefault();

if (debug)
debug->append(TString::Format(" Argument:%s Type:%s Value:%s \n", arg->GetName(), arg->GetFullTypeName(),
val ? val : "<missed>")
.Data());
if (!val)
return debug != nullptr;
debug->append(
TString::Format(" Argument:%s Type:%s Value:%s \n", arg->GetName(), arg->GetFullTypeName(), sval.Data())
.Data());

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);
Bool_t isstr = (strcmp(arg->GetFullTypeName(), "const char*") == 0) ||
(strcmp(arg->GetFullTypeName(), "Option_t*") == 0) ||
(strcmp(arg->GetFullTypeName(), "string") == 0);

if (isstr) {
// check that quotes provided for the string argument
// all special characters were escaped before
if (sval.IsNull())
sval = "\"\"";
else {
if (sval[0] != '"')
sval.Prepend("\"");
if (sval[sval.Length() - 1] != '"')
sval.Append("\"");
}
} else {
call_args.Append(val);
// for numeric types keep only numeric and alphabetic characters
// exclude others - especially remove all escape characters
if (sanitize_numeric) {
TString sanitized;
for(Size_t i = 0; i < sval.Length(); ++i) {
if (std::isalnum(sval[i]) || std::strchr(".:+-", sval[i]))
sanitized.Append(sval[i]);
}
sval = sanitized;
}
if (sval.IsNull())
sval = "0";
}

call_args.Append(sval);
}

TMethodCall *call = nullptr;
Expand Down
12 changes: 12 additions & 0 deletions net/httpsniff/test/CMakeLists.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,12 @@
# Copyright (C) 1995-2026, Rene Brun and Fons Rademakers.
# All rights reserved.
#
# For the licensing terms see $ROOTSYS/LICENSE.
# For the list of contributors see $ROOTSYS/README/CREDITS.

############################################################################
# CMakeLists.txt file for building ROOT net/http package
# @author Sergey Linev, GSI
############################################################################

ROOT_ADD_GTEST(testRootSniffer test_sniffer.cxx LIBRARIES RHTTPSniff)
Loading
Loading