From 55030e6140238bd389e0bf9874b6815299729739 Mon Sep 17 00:00:00 2001 From: Brian Bockelman Date: Mon, 25 Mar 2019 20:40:20 -0500 Subject: [PATCH 1/3] Set the `macaroons.onmissing` default. Without this, if the `onmissing` parameter is not set, it results in undefined behavior. Works on developer testbeds, blows up in production. Issue originally reported by John Thiltges. --- src/XrdMacaroons/XrdMacaroonsAuthz.cc | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/XrdMacaroons/XrdMacaroonsAuthz.cc b/src/XrdMacaroons/XrdMacaroonsAuthz.cc index c0d474ffa21..76e90cc4f08 100644 --- a/src/XrdMacaroons/XrdMacaroonsAuthz.cc +++ b/src/XrdMacaroons/XrdMacaroonsAuthz.cc @@ -111,7 +111,7 @@ Authz::Authz(XrdSysLogger *log, char const *config, XrdAccAuthorize *chain) m_log(log, "macarons_"), m_authz_behavior(static_cast(Handler::AuthzBehavior::PASSTHROUGH)) { - Handler::AuthzBehavior behavior; + Handler::AuthzBehavior behavior(Handler::AuthzBehavior::PASSTHROUGH); if (!Handler::Config(config, nullptr, &m_log, m_location, m_secret, m_max_duration, behavior)) { throw std::runtime_error("Macaroon authorization config failed."); From b85bccc7445a775cd480d40ec44f23482c7a6d38 Mon Sep 17 00:00:00 2001 From: Brian Bockelman Date: Mon, 25 Mar 2019 20:42:06 -0500 Subject: [PATCH 2/3] Interact with json-c appropriately for request. This commit prevents a potential read of uninitialized bytes (not triggered under normal circumstances but flagged by valgrind) and ensures that we free the resulting JSON object. --- src/XrdMacaroons/XrdMacaroonsHandler.cc | 20 +++++++++++++++++--- 1 file changed, 17 insertions(+), 3 deletions(-) diff --git a/src/XrdMacaroons/XrdMacaroonsHandler.cc b/src/XrdMacaroons/XrdMacaroonsHandler.cc index 33e1dde5e85..18f4a98382d 100644 --- a/src/XrdMacaroons/XrdMacaroonsHandler.cc +++ b/src/XrdMacaroons/XrdMacaroonsHandler.cc @@ -366,30 +366,44 @@ int Handler::ProcessReq(XrdHttpExtReq &req) return req.SendSimpleResp(400, NULL, NULL, "Content-Length has invalid value.", 0); } //for (const auto &header : req.headers) { printf("** Request header: %s=%s\n", header.first.c_str(), header.second.c_str()); } + + // request_data is not necessarily null-terminated; hence, we use the more advanced _ex variant + // of the tokener to avoid making a copy of the character buffer. char *request_data; if (req.BuffgetData(blen, &request_data, true) != blen) { return req.SendSimpleResp(400, NULL, NULL, "Missing or invalid body of request.", 0); } - json_object *macaroon_req = json_tokener_parse(request_data); - if (!macaroon_req) + json_tokener *tokener = json_tokener_new(); + if (!tokener) + { + return req.SendSimpleResp(500, NULL, NULL, "Internal error when allocating token parser.", 0); + } + json_object *macaroon_req = json_tokener_parse_ex(tokener, request_data, blen); + enum json_tokener_error err = json_tokener_get_error(tokener); + json_tokener_free(tokener); + if (err != json_tokener_success) { + if (macaroon_req) json_object_put(macaroon_req); return req.SendSimpleResp(400, NULL, NULL, "Invalid JSON serialization of macaroon request.", 0); } json_object *validity_obj; if (!json_object_object_get_ex(macaroon_req, "validity", &validity_obj)) { + json_object_put(macaroon_req); return req.SendSimpleResp(400, NULL, NULL, "JSON request does not include a `validity`", 0); } const char *validity_cstr = json_object_get_string(validity_obj); if (!validity_cstr) { + json_object_put(macaroon_req); return req.SendSimpleResp(400, NULL, NULL, "validity key cannot be cast to a string", 0); } std::string validity_str(validity_cstr); ssize_t validity = determine_validity(validity_str); if (validity <= 0) { + json_object_put(macaroon_req); return req.SendSimpleResp(400, NULL, NULL, "Invalid ISO 8601 duration for validity key", 0); } json_object *caveats_obj; @@ -412,7 +426,7 @@ int Handler::ProcessReq(XrdHttpExtReq &req) } } } - json_object_put(caveats_obj); + json_object_put(macaroon_req); return GenerateMacaroonResponse(req, req.resource, other_caveats, validity, false); } From f3d16ffe7989f85d858f68f31fc976047d03ead4 Mon Sep 17 00:00:00 2001 From: Brian Bockelman Date: Mon, 25 Mar 2019 20:43:13 -0500 Subject: [PATCH 3/3] Ensure we delete macaroon object after use. --- src/XrdMacaroons/XrdMacaroonsHandler.cc | 1 + 1 file changed, 1 insertion(+) diff --git a/src/XrdMacaroons/XrdMacaroonsHandler.cc b/src/XrdMacaroons/XrdMacaroonsHandler.cc index 18f4a98382d..323b666d708 100644 --- a/src/XrdMacaroons/XrdMacaroonsHandler.cc +++ b/src/XrdMacaroons/XrdMacaroonsHandler.cc @@ -540,6 +540,7 @@ Handler::GenerateMacaroonResponse(XrdHttpExtReq &req, const std::string &resourc printf("Returned macaroon_serialize code: %lu\n", (unsigned long)size_hint); return req.SendSimpleResp(500, NULL, NULL, "Internal error serializing macaroon", 0); } + macaroon_destroy(mac_with_date); json_object *response_obj = json_object_new_object(); if (!response_obj)