Misc OAuth/JOSE/Crypto/OIDC hardening (#2689)

Signed-off-by: Juan Cruz Viotti <jv@jviotti.com>

Author: jviotti

src/core/crypto/crypto_pkcs8.h

@@ -61,6 +61,60 @@ struct PKCS8Key {
   bool rsa_pss_restricted{false};
 };
 
+// Whether a PKCS#1 RSAPrivateKey carries usable components (RFC 8017 Appendix
+// A.1.2). The modulus and both exponents are decoded as canonical non-negative
+// DER INTEGERs, so a negative or non-canonically encoded one cannot be silently
+// reinterpreted as a different positive number and signed with, and the public
+// exponent is range-checked per RFC 8017 Section 3.1. The version is required
+// to be present as an INTEGER but its value is left to the backends, which
+// differ on whether they accept the multi-prime form. Checked here rather than
+// in each backend, since only the reference one reads these components itself
+// and the rest hand the blob straight to a platform that does not apply the
+// rule
+inline auto rsa_private_key_acceptable(const std::string_view key) -> bool {
+  // A canonical RSAPrivateKey is exactly one SEQUENCE, so bytes trailing it
+  // mark a malformed encoding (X.690 Section 10.1), the same rule the enclosing
+  // PrivateKeyInfo is held to
+  const auto sequence{der_read(key)};
+  if (!sequence.has_value() || sequence->tag != 0x30 ||
+      !sequence->rest.empty()) {
+    return false;
+  }
+
+  const auto version{der_read(sequence->content)};
+  if (!version.has_value() || version->tag != 0x02) {
+    return false;
+  }
+
+  const auto modulus{der_read(version->rest)};
+  if (!modulus.has_value() || modulus->tag != 0x02) {
+    return false;
+  }
+
+  const auto public_exponent{der_read(modulus->rest)};
+  if (!public_exponent.has_value() || public_exponent->tag != 0x02) {
+    return false;
+  }
+
+  const auto private_exponent{der_read(public_exponent->rest)};
+  if (!private_exponent.has_value() || private_exponent->tag != 0x02) {
+    return false;
+  }
+
+  const auto modulus_value{der_unsigned_integer(modulus->content)};
+  const auto public_exponent_value{
+      der_unsigned_integer(public_exponent->content)};
+  const auto private_exponent_value{
+      der_unsigned_integer(private_exponent->content)};
+  return modulus_value.has_value() && public_exponent_value.has_value() &&
+         private_exponent_value.has_value() && !modulus_value->empty() &&
+         !private_exponent_value->empty() &&
+         modulus_value->size() <= MAXIMUM_KEY_BYTES &&
+         private_exponent_value->size() <= MAXIMUM_KEY_BYTES &&
+         rsa_public_exponent_acceptable(public_exponent_value.value(),
+                                        modulus_value.value());
+}
+
 // Parse an RFC 5958 PrivateKeyInfo, identifying the algorithm from its object
 // identifier and returning the raw privateKey octets
 inline auto parse_pkcs8(const std::string_view der) -> std::optional<PKCS8Key> {
@@ -106,6 +160,10 @@ inline auto parse_pkcs8(const std::string_view der) -> std::optional<PKCS8Key> {
   // NOLINTEND(modernize-raw-string-literal)
 
   if (oid->content == rsa || oid->content == rsa_pss) {
+    if (!rsa_private_key_acceptable(private_key->content)) {
+      return std::nullopt;
+    }
+
     return PKCS8Key{.kind = PKCS8KeyKind::RSA,
                     .curve = {},
                     .edwards_curve = {},

src/core/crypto/crypto_sign_windows.cc

@@ -119,13 +119,20 @@ auto native_rsa_private_key(const std::string_view rsa_private_key) -> KeyPair {
       return {.algorithm = nullptr, .key = nullptr};
     }
 
-    field = sourcemeta::core::strip_left(element->content, '\x00');
+    // Decoded as a canonical non-negative DER INTEGER rather than merely having
+    // its leading zeros stripped, so a value whose leading octet sets the high
+    // bit, which X.690 Section 8.3 makes negative, cannot be read as a large
+    // positive one
+    const auto value{sourcemeta::core::der_unsigned_integer(element->content)};
     // Every field feeds a blob length or padding width, so oversize input is
     // rejected before it drives a cast or an allocation
-    if (field.size() > sourcemeta::core::MAXIMUM_KEY_BYTES) {
+    if (!value.has_value() ||
+        value.value().size() > sourcemeta::core::MAXIMUM_KEY_BYTES) {
       return {.algorithm = nullptr, .key = nullptr};
     }
 
+    field = value.value();
+
     rest = element->rest;
   }
 

src/core/jose/include/sourcemeta/core/jose_verify.h

@@ -79,6 +79,32 @@ struct JWTClockSkew {
   std::chrono::seconds issued_at{0};
 };
 
+/// @ingroup jose
+/// Reduce a caller-supplied clock skew to the grace period token validation
+/// honours, one mean Gregorian year, treating a negative tolerance as none.
+/// Every path that validates a token applies this, so the same tolerance means
+/// the same thing whichever one runs. For example:
+///
+/// ```cpp
+/// #include <sourcemeta/core/jose.h>
+/// #include <chrono>
+/// #include <cassert>
+///
+/// assert(sourcemeta::core::jwt_bounded_clock_skew(
+///            std::chrono::seconds{30}) == std::chrono::seconds{30});
+/// assert(sourcemeta::core::jwt_bounded_clock_skew(
+///            std::chrono::seconds{-30}) == std::chrono::seconds{0});
+/// ```
+inline auto jwt_bounded_clock_skew(const std::chrono::seconds skew) noexcept
+    -> std::chrono::seconds {
+  // A mean Gregorian year, the widest grace period any deployment plausibly
+  // needs, so an extreme value cannot widen the acceptance window without bound
+  constexpr std::chrono::seconds maximum{31556952};
+  return skew < std::chrono::seconds::zero() ? std::chrono::seconds::zero()
+         : skew > maximum                    ? maximum
+                                             : skew;
+}
+
 /// @ingroup jose
 /// Validate the registered claims of a JSON Web Token against the expected
 /// issuer and audience at a given time, returning the first failing check or no

src/core/jose/jose_jwt_check_claims.cc

@@ -1,6 +1,7 @@
 #include <sourcemeta/core/jose_verify.h>
 
-#include <algorithm>   // std::clamp
+#include <sourcemeta/core/time.h>
+
 #include <chrono>      // std::chrono::seconds, std::chrono::system_clock
 #include <optional>    // std::optional, std::nullopt
 #include <string_view> // std::string_view
@@ -11,32 +12,19 @@ using Clock = std::chrono::system_clock;
 
 // The skew is applied to the server clock rather than the attacker-controlled
 // claim, so a NumericDate near the representable bound cannot overflow the
-// comparison (the two forms are otherwise equivalent). The skew is also
-// clamped to a non-negative, bounded grace period and the shift saturates, so
-// an extreme caller-supplied clock or skew stays well-defined too
-auto skew_ticks(const std::chrono::seconds skew) -> Clock::duration {
-  return std::chrono::duration_cast<Clock::duration>(std::clamp(
-      skew, std::chrono::seconds::zero(), std::chrono::seconds{31556952}));
-}
-
+// comparison (the two forms are otherwise equivalent). The shift itself
+// saturates, and the skew is bounded first, so an extreme caller-supplied clock
+// or skew stays well-defined too
 auto shift_backward(const Clock::time_point now,
                     const std::chrono::seconds skew) -> Clock::time_point {
-  const auto ticks{skew_ticks(skew)};
-  if (now.time_since_epoch() < Clock::duration::min() + ticks) {
-    return Clock::time_point{Clock::duration::min()};
-  }
-
-  return now - ticks;
+  return sourcemeta::core::clock_shift_backward(
+      now, sourcemeta::core::jwt_bounded_clock_skew(skew));
 }
 
 auto shift_forward(const Clock::time_point now, const std::chrono::seconds skew)
     -> Clock::time_point {
-  const auto ticks{skew_ticks(skew)};
-  if (now.time_since_epoch() > Clock::duration::max() - ticks) {
-    return Clock::time_point{Clock::duration::max()};
-  }
-
-  return now + ticks;
+  return sourcemeta::core::clock_shift_forward(
+      now, sourcemeta::core::jwt_bounded_clock_skew(skew));
 }
 
 } // namespace

src/core/oauth/CMakeLists.txt

@@ -26,6 +26,8 @@ target_link_libraries(sourcemeta_core_oauth
   PRIVATE sourcemeta::core::text)
 target_link_libraries(sourcemeta_core_oauth
   PRIVATE sourcemeta::core::html)
+target_link_libraries(sourcemeta_core_oauth
+  PRIVATE sourcemeta::core::time)
 
 if(SOURCEMETA_CORE_INSTALL)
   sourcemeta_library_install(NAMESPACE sourcemeta PROJECT core NAME oauth)

src/core/oauth/include/sourcemeta/core/oauth_metadata.h

@@ -117,6 +117,10 @@ class SOURCEMETA_CORE_OAUTH_EXPORT OAuthServerMetadata {
   [[nodiscard]] auto registration_endpoint() const
       -> std::optional<std::string_view>;
 
+  /// The device authorization endpoint (RFC 8628 Section 4).
+  [[nodiscard]] auto device_authorization_endpoint() const
+      -> std::optional<std::string_view>;
+
   /// The token revocation endpoint (RFC 8414 Section 2).
   [[nodiscard]] auto revocation_endpoint() const
       -> std::optional<std::string_view>;

src/core/oauth/oauth_dpop.cc

@@ -8,6 +8,7 @@
 #include <sourcemeta/core/jose_verify.h>
 #include <sourcemeta/core/json.h>
 #include <sourcemeta/core/oauth_random.h>
+#include <sourcemeta/core/time.h>
 
 #include "oauth_json.h"
 #include "oauth_syntax.h"
@@ -29,27 +30,6 @@ namespace {
 
 using namespace std::literals::string_view_literals;
 
-// When an entry stops guarding against a replay, saturating at the newest
-// instant the clock can express. A caller derives the window from a token
-// lifetime, so the value is attacker-influenced and adding it to the clock
-// unchecked would overflow, wrap negative, and have the entry pruned on the
-// very next call, silently disabling the replay guard it was inserted for
-[[maybe_unused]] auto
-replay_expiry(const std::chrono::system_clock::time_point now,
-              const std::chrono::seconds window)
-    -> std::chrono::system_clock::time_point {
-  using Clock = std::chrono::system_clock;
-  constexpr auto limit{
-      std::chrono::duration_cast<std::chrono::seconds>(Clock::duration::max())};
-  const auto ticks{std::chrono::duration_cast<Clock::duration>(
-      std::clamp(window, std::chrono::seconds::zero(), limit))};
-  if (now.time_since_epoch() > Clock::duration::max() - ticks) {
-    return Clock::time_point{Clock::duration::max()};
-  }
-
-  return now + ticks;
-}
-
 constexpr auto HASH_TYP{JSON::Object::hash("typ"sv)};
 constexpr auto HASH_ALG{JSON::Object::hash("alg"sv)};
 constexpr auto HASH_JWK{JSON::Object::hash("jwk"sv)};
@@ -343,8 +323,8 @@ auto oauth_dpop_verify(const std::string_view proof,
   }
 
   // Check 11: the creation time is within the acceptable window
-  if (issued.value() < now - options.past_window ||
-      issued.value() > now + options.future_window) {
+  if (issued.value() < clock_shift_backward(now, options.past_window) ||
+      issued.value() > clock_shift_forward(now, options.future_window)) {
     return OAuthDPoPError::Expired;
   }
 
@@ -425,7 +405,7 @@ auto OAuthDPoPReplayStore::check_and_insert(
   }
 
   this->entries_.push_back(
-      Entry{.digest = digest, .expiry = replay_expiry(now, window)});
+      Entry{.digest = digest, .expiry = clock_shift_forward(now, window)});
   return true;
 }
 

src/core/oauth/oauth_metadata.cc

@@ -357,6 +357,12 @@ auto OAuthServerMetadata::registration_endpoint() const
                        HASH_REGISTRATION_ENDPOINT);
 }
 
+auto OAuthServerMetadata::device_authorization_endpoint() const
+    -> std::optional<std::string_view> {
+  return string_member(this->data_, "device_authorization_endpoint"sv,
+                       HASH_DEVICE_AUTHORIZATION_ENDPOINT);
+}
+
 auto OAuthServerMetadata::revocation_endpoint() const
     -> std::optional<std::string_view> {
   return string_member(this->data_, "revocation_endpoint"sv,

src/core/oidc/CMakeLists.txt

@@ -4,7 +4,7 @@ sourcemeta_library(NAMESPACE sourcemeta PROJECT core NAME oidc
     request_object.h logout.h encryption.h profile.h
   SOURCES oidc_error.cc oidc_metadata.cc oidc_discovery.cc oidc_hash.cc
     oidc_id_token.cc oidc_authentication.cc oidc_claims.cc oidc_userinfo.cc
-    oidc_registration.cc oidc_subject.cc oidc_verify.h oidc_time.h
+    oidc_registration.cc oidc_subject.cc oidc_verify.h
     oidc_request_object.cc oidc_logout.cc oidc_encryption.cc)
 
 target_link_libraries(sourcemeta_core_oidc

src/core/oidc/oidc_id_token.cc

@@ -6,8 +6,6 @@
 #include <sourcemeta/core/oidc_hash.h>
 #include <sourcemeta/core/time.h>
 
-#include "oidc_time.h"
-
 #include <chrono> // std::chrono::system_clock, std::chrono::seconds, std::chrono::duration, std::chrono::duration_cast
 #include <cstdint>     // std::int64_t
 #include <optional>    // std::optional, std::nullopt
@@ -106,7 +104,7 @@ auto oidc_id_token_checks(const JWT &token, const std::string_view issuer,
   // policy
   if (options.maximum_issued_at_age.has_value() &&
       issued_at.value() <
-          oidc_shift_backward(now, options.maximum_issued_at_age.value())) {
+          clock_shift_backward(now, options.maximum_issued_at_age.value())) {
     return std::nullopt;
   }
 
@@ -154,8 +152,8 @@ auto oidc_id_token_checks(const JWT &token, const std::string_view issuer,
     // satisfy a freshness window and is rejected before the age comparison
     if (!authentication_time.has_value() || authentication_time.value() > now ||
         authentication_time.value() <
-            oidc_shift_backward(now,
-                                options.maximum_authentication_age.value())) {
+            clock_shift_backward(now,
+                                 options.maximum_authentication_age.value())) {
       return std::nullopt;
     }
   }