Skip to content

Commit

Permalink
XrdHttp: obfuscate token authz leaking to the logs
Browse files Browse the repository at this point in the history
  • Loading branch information
ccaffy authored and apeters1971 committed Jun 19, 2024
1 parent 3e884a9 commit 8805939
Show file tree
Hide file tree
Showing 7 changed files with 99 additions and 5 deletions.
10 changes: 7 additions & 3 deletions src/XrdHttp/XrdHttpProtocol.cc
Original file line number Diff line number Diff line change
Expand Up @@ -42,6 +42,7 @@

#include "XrdTls/XrdTls.hh"
#include "XrdTls/XrdTlsContext.hh"
#include "XrdOuc/XrdOucUtils.hh"

#include <openssl/err.h>
#include <openssl/ssl.h>
Expand All @@ -51,6 +52,7 @@
#include <cctype>
#include <sys/stat.h>
#include <fcntl.h>
#include <algorithm>

#define XRHTTP_TK_GRACETIME 600

Expand Down Expand Up @@ -313,7 +315,6 @@ char *XrdHttpProtocol::GetClientIPStr() {
return strdup(buf);
}


// Various routines for handling XrdLink as BIO objects within OpenSSL.
#if OPENSSL_VERSION_NUMBER < 0x1000105fL
int BIO_XrdLink_write(BIO *bio, const char *data, size_t datal, size_t *written)
Expand Down Expand Up @@ -615,8 +616,11 @@ int XrdHttpProtocol::Process(XrdLink *lp) // We ignore the argument here

// Read as many lines as possible into the buffer. An empty line breaks
while ((rc = BuffgetLine(tmpline)) > 0) {
TRACE(DEBUG, " rc:" << rc << " got hdr line: " << tmpline.c_str());

if (TRACING(TRACE_DEBUG)) {
std::string traceLine{tmpline.c_str()};
traceLine = XrdOucUtils::obfuscate(traceLine, {"authorization", "transferheaderauthorization"}, ':', '\n');
TRACE(DEBUG, " rc:" << rc << " got hdr line: " << traceLine);
}
if ((rc == 2) && (tmpline.length() > 1) && (tmpline[rc - 1] == '\n')) {
CurrentReq.headerok = true;
TRACE(DEBUG, " rc:" << rc << " detected header end.");
Expand Down
10 changes: 8 additions & 2 deletions src/XrdHttp/XrdHttpReq.cc
Original file line number Diff line number Diff line change
Expand Up @@ -945,9 +945,15 @@ int XrdHttpReq::ProcessHTTPReq() {

char *q = quote(hdr2cgistr.c_str());
resourceplusopaque.append(q);
TRACEI(DEBUG, "Appended header fields to opaque info: '"
<< hdr2cgistr.c_str() << "'");
if (TRACING(TRACE_DEBUG)) {
// The obfuscation of "authz" will only be done if the server http.header2cgi config contains something that maps a header to this "authz" cgi.
// Unfortunately the obfuscation code will be called no matter what is configured in http.header2cgi.
std::string header2cgistrObf = XrdOucUtils::obfuscate(hdr2cgistr, {"authz"}, '=', '&');

TRACEI(DEBUG, "Appended header fields to opaque info: '"
<< header2cgistrObf.c_str() << "'");

}
// We assume that anything appended to the CGI str should also
// apply to the destination in case of a MOVE.
if (strchr(destination.c_str(), '?')) destination.append("&");
Expand Down
44 changes: 44 additions & 0 deletions src/XrdOuc/XrdOucUtils.cc
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,8 @@
#include <cstdio>
#include <list>
#include <vector>
#include <unordered_set>
#include <algorithm>

#ifdef WIN32
#include <direct.h>
Expand Down Expand Up @@ -1409,5 +1411,47 @@ void XrdOucUtils::trim(std::string &str) {
while( str.size() && !isgraph(str[str.size()-1]) )
str.resize (str.size () - 1);
}

std::string XrdOucUtils::obfuscate(const std::string & input, const std::unordered_set<std::string> & keysToObfuscate,const char keyValueDelimiter, const char listDelimiter) {
// String stream to build the result
std::ostringstream result;

// Start processing the input string
std::istringstream inputStream(input);
std::string pair;
bool first = true;

// Split input by listDelimiter
while (std::getline(inputStream, pair, listDelimiter)) {
if (!first) {
result << listDelimiter;
} else {
first = false;
}

// Find the position of the keyValueDelimiter
size_t delimiterPos = pair.find(keyValueDelimiter);
if (delimiterPos != std::string::npos) {
std::string key = pair.substr(0, delimiterPos);
trim(key);
std::string lowerCasedKey = key;
std::transform(lowerCasedKey.begin(),lowerCasedKey.end(),lowerCasedKey.begin(),::tolower);
std::string value = pair.substr(delimiterPos + 1);

// Check if the key needs to be obfuscated
if (keysToObfuscate.find(lowerCasedKey) != keysToObfuscate.end()) {
result << key << keyValueDelimiter << OBFUSCATION_STR;
} else {
result << pair;
}
} else {
result << pair; // In case there's no delimiter in the pair, just append it
}
}

return result.str();
}


#endif

5 changes: 5 additions & 0 deletions src/XrdOuc/XrdOucUtils.hh
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,7 @@
#include <sys/types.h>
#include <sys/stat.h>
#include <string>
#include <unordered_set>

class XrdSysError;
class XrdOucString;
Expand All @@ -42,6 +43,8 @@ class XrdOucUtils
{
public:

static inline std::string OBFUSCATION_STR = "REDACTED";

static const mode_t pathMode = S_IRWXU|S_IRGRP|S_IXGRP|S_IROTH|S_IXOTH;

static int argList(char *args, char **argV, int argC);
Expand Down Expand Up @@ -133,6 +136,8 @@ static int getModificationTime(const char * path, time_t & modificationTime);

static void trim(std::string & str);

static std::string obfuscate(const std::string & input, const std::unordered_set<std::string> & keysToObfuscate,const char keyValueDelimiter, const char listDelimiter);

XrdOucUtils() {}
~XrdOucUtils() {}
};
Expand Down
2 changes: 2 additions & 0 deletions tests/CMakeLists.txt
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,8 @@ add_subdirectory(XrdEc)

add_subdirectory(XrdHttpTests)

add_subdirectory(XrdOucTests)

add_subdirectory( XrdSsiTests )

if(NOT ENABLE_SERVER_TESTS)
Expand Down
6 changes: 6 additions & 0 deletions tests/XrdOucTests/CMakeLists.txt
Original file line number Diff line number Diff line change
@@ -0,0 +1,6 @@
add_executable(xrdoucutils-unit-tests XrdOucUtilsTests.cc)

target_link_libraries(xrdoucutils-unit-tests XrdUtils GTest::GTest GTest::Main)
target_include_directories(xrdoucutils-unit-tests PRIVATE ${CMAKE_SOURCE_DIR}/src)

gtest_discover_tests(xrdoucutils-unit-tests)
27 changes: 27 additions & 0 deletions tests/XrdOucTests/XrdOucUtilsTests.cc
Original file line number Diff line number Diff line change
@@ -0,0 +1,27 @@
#undef NDEBUG

#include <gtest/gtest.h>
#include <string>

#include "XrdOuc/XrdOucUtils.hh"


using namespace testing;

class XrdOucUtilsTests : public Test {};

TEST(XrdOucUtilsTests, obfuscate) {
// General cases
ASSERT_EQ(std::string("scitag.flow=144&authz=") + XrdOucUtils::OBFUSCATION_STR + std::string("&test=abcd"), XrdOucUtils::obfuscate("scitag.flow=144&authz=token&test=abcd",{"authz"},'=','&'));
ASSERT_EQ(std::string("authz=") + XrdOucUtils::OBFUSCATION_STR + std::string("&scitag.flow=144&test=abcd"), XrdOucUtils::obfuscate("authz=token&scitag.flow=144&test=abcd",{"authz"},'=','&'));
ASSERT_EQ(std::string("scitag.flow=144&test=abcd&authz=") + XrdOucUtils::OBFUSCATION_STR, XrdOucUtils::obfuscate("scitag.flow=144&test=abcd&authz=token",{"authz"},'=','&'));
// Nothing to obfuscate
ASSERT_EQ("test=abcd&test2=abcde",XrdOucUtils::obfuscate("test=abcd&test2=abcde",{},'=','&'));
ASSERT_EQ("nothingtoobfuscate",XrdOucUtils::obfuscate("nothingtoobfuscate",{"obfuscateme"},'=','\n'));
//Empty string obfuscation
ASSERT_EQ("",XrdOucUtils::obfuscate("",{"obfuscateme"},'=','&'));
ASSERT_EQ("",XrdOucUtils::obfuscate("",{},'=','\n'));
// Trimmed key obfuscation
ASSERT_EQ(std::string("Authorization:") + XrdOucUtils::OBFUSCATION_STR, XrdOucUtils::obfuscate("Authorization: Bearer token",{"authorization"},':','\n'));
ASSERT_EQ(std::string("Authorization:")+ XrdOucUtils::OBFUSCATION_STR, XrdOucUtils::obfuscate("Authorization : Bearer token",{"authorization"},':','\n'));
}

0 comments on commit 8805939

Please sign in to comment.