Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
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: 2 additions & 2 deletions .github/workflows/checks.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -113,7 +113,7 @@ jobs:

- name: Check out and start up platform with deps/containers
id: run-platform
uses: opentdf/platform/test/start-up-with-containers@main
uses: opentdf/platform/test/start-up-with-containers@fix/start-additional-kas-workflow
with:
platform-ref: main

Expand Down Expand Up @@ -240,7 +240,7 @@ jobs:
working-directory: cmdline

- name: Start additional kas
uses: opentdf/platform/test/start-additional-kas@main
uses: opentdf/platform/test/start-additional-kas@fix/start-additional-kas-workflow
with:
kas-port: 8282
kas-name: beta
Expand Down
48 changes: 44 additions & 4 deletions cmdline/src/main/java/io/opentdf/platform/Command.java
Original file line number Diff line number Diff line change
@@ -1,6 +1,16 @@
package io.opentdf.platform;

import com.google.gson.Gson;
import com.google.gson.JsonDeserializationContext;
import com.google.gson.JsonDeserializer;
import com.google.gson.JsonElement;
import com.google.gson.JsonObject;
import com.google.gson.JsonParseException;
import com.nimbusds.jose.jwk.JWK;
import com.google.gson.GsonBuilder;
import com.google.gson.reflect.TypeToken;

import java.text.ParseException;
import com.google.gson.JsonSyntaxException;
import com.nimbusds.jose.JOSEException;
import io.opentdf.platform.sdk.AssertionConfig;
Expand Down Expand Up @@ -57,8 +67,38 @@ class Versions {
+ "\",\"tdfSpecVersion\":\"" + Versions.TDF_SPEC + "\"}")
class Command {

@Option(names = { "-V", "--version" }, versionHelp = true, description = "display version info")
boolean versionInfoRequested;
private static class AssertionKeyDeserializer implements JsonDeserializer<AssertionConfig.AssertionKey> {
@Override
public AssertionConfig.AssertionKey deserialize(JsonElement json, java.lang.reflect.Type typeOfT, JsonDeserializationContext context) throws JsonParseException {
JsonObject jsonObject = json.getAsJsonObject();
AssertionConfig.AssertionKey assertionKey = new AssertionConfig.AssertionKey(AssertionConfig.AssertionKeyAlg.NotDefined, null);

if (jsonObject.has("alg")) {
assertionKey.alg = context.deserialize(jsonObject.get("alg"), AssertionConfig.AssertionKeyAlg.class);
}
if (jsonObject.has("key")) {
assertionKey.key = context.deserialize(jsonObject.get("key"), Object.class);
}
if (jsonObject.has("jwk")) {
try {
assertionKey.jwk = JWK.parse(jsonObject.get("jwk").toString());
} catch (ParseException e) {
throw new JsonParseException("Failed to parse jwk", e);
}
}
if (jsonObject.has("x5c")) {
assertionKey.x5c = context.deserialize(jsonObject.get("x5c"), new TypeToken<List<com.nimbusds.jose.util.Base64>>() {}.getType());
}

return assertionKey;
}
}

private Gson buildGson() {
return new GsonBuilder()
.registerTypeAdapter(AssertionConfig.AssertionKey.class, new AssertionKeyDeserializer())
.create();
}

private static final String PRIVATE_KEY_HEADER = "-----BEGIN PRIVATE KEY-----";
private static final String PRIVATE_KEY_FOOTER = "-----END PRIVATE KEY-----";
Expand Down Expand Up @@ -177,7 +217,7 @@ void encrypt(

if (assertion.isPresent()) {
var assertionConfig = assertion.get();
Gson gson = new Gson();
Gson gson = buildGson();

AssertionConfig[] assertionConfigs;
try {
Expand Down Expand Up @@ -252,7 +292,7 @@ void decrypt(@Option(names = { "-f", "--file" }, required = true) Path tdfPath,
try (var stdout = new BufferedOutputStream(System.out)) {
if (assertionVerification.isPresent()) {
var assertionVerificationInput = assertionVerification.get();
Gson gson = new Gson();
Gson gson = buildGson();

AssertionVerificationKeys assertionVerificationKeys;
try {
Expand Down
15 changes: 15 additions & 0 deletions sdk/src/main/java/io/opentdf/platform/sdk/AssertionConfig.java
Original file line number Diff line number Diff line change
Expand Up @@ -2,11 +2,14 @@

import com.google.gson.Gson;
import com.google.gson.annotations.SerializedName;
import com.nimbusds.jose.jwk.JWK;
import com.nimbusds.jose.util.Base64;

import java.net.InetAddress;
import java.net.UnknownHostException;
import java.time.OffsetDateTime;
import java.time.format.DateTimeFormatter;
import java.util.List;
import java.util.Objects;

/**
Expand Down Expand Up @@ -88,12 +91,24 @@ public String toString() {
static public class AssertionKey {
public Object key;
public AssertionKeyAlg alg = AssertionKeyAlg.NotDefined;
public transient JWK jwk;
public transient List<Base64> x5c;

public AssertionKey(AssertionKeyAlg alg, Object key) {
this.alg = alg;
this.key = key;
}

public AssertionKey withJwk(JWK jwk) {
this.jwk = jwk;
return this;
}

public AssertionKey withX5c(List<Base64> x5c) {
this.x5c = x5c;
return this;
}

public boolean isDefined() {
return alg != AssertionKeyAlg.NotDefined;
}
Expand Down
12 changes: 12 additions & 0 deletions sdk/src/main/java/io/opentdf/platform/sdk/CryptoUtils.java
Original file line number Diff line number Diff line change
@@ -1,8 +1,11 @@
package io.opentdf.platform.sdk;

import com.nimbusds.jose.jwk.RSAKey;

import javax.crypto.Mac;
import javax.crypto.spec.SecretKeySpec;
import java.security.*;
import java.security.interfaces.RSAPublicKey;
import java.security.spec.ECGenParameterSpec;
import java.util.Base64;

Expand Down Expand Up @@ -58,6 +61,15 @@ public static String getPublicKeyPEM(PublicKey publicKey) {
"\r\n-----END PUBLIC KEY-----";
}

public static String getPublicKeyJWK(PublicKey publicKey) {
if (publicKey instanceof RSAPublicKey) {
RSAKey jwk = new RSAKey.Builder((RSAPublicKey) publicKey).build();
return jwk.toString();
} else {
throw new IllegalArgumentException("Unsupported public key algorithm: " + publicKey.getAlgorithm());
}
}

public static String getPrivateKeyPEM(PrivateKey privateKey) {
return "-----BEGIN PRIVATE KEY-----\r\n" +
Base64.getMimeEncoder().encodeToString(privateKey.getEncoded()) +
Expand Down
73 changes: 66 additions & 7 deletions sdk/src/main/java/io/opentdf/platform/sdk/Manifest.java
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,9 @@
import com.nimbusds.jose.crypto.MACVerifier;
import com.nimbusds.jose.crypto.RSASSASigner;
import com.nimbusds.jose.crypto.RSASSAVerifier;
import com.nimbusds.jose.crypto.factories.DefaultJWSVerifierFactory;
import com.nimbusds.jose.jwk.JWK;
import com.nimbusds.jose.util.X509CertUtils;
import com.nimbusds.jwt.JWTClaimsSet;
import com.nimbusds.jwt.SignedJWT;
import io.opentdf.platform.sdk.SDK.AssertionException;
Expand All @@ -33,6 +36,7 @@
import java.security.NoSuchAlgorithmException;
import java.security.PrivateKey;
import java.security.interfaces.RSAPublicKey;
import java.security.cert.X509Certificate;
import java.text.ParseException;
import java.util.ArrayList;
import java.util.Base64;
Expand Down Expand Up @@ -400,7 +404,7 @@ public void sign(final HashValues hashValues, final AssertionConfig.AssertionKey
// returns the hash and the signature. It returns an error if the verification
// fails.
public Assertion.HashValues verify(AssertionConfig.AssertionKey assertionKey)
throws ParseException, JOSEException {
throws ParseException, JOSEException, java.security.cert.CertificateException {
if (binding == null) {
throw new AssertionException("Binding is null in assertion", this.id);
}
Expand All @@ -409,7 +413,37 @@ public Assertion.HashValues verify(AssertionConfig.AssertionKey assertionKey)
binding = null; // Clear the binding after use

SignedJWT signedJWT = SignedJWT.parse(signatureString);
JWSVerifier verifier = createVerifier(assertionKey);
JWSHeader header = signedJWT.getHeader();
JWSVerifier verifier = null;

// Check for JWK in header
if (header.getJWK() != null) {
try {
verifier = createVerifier(header.getJWK());
} catch (JOSEException e) {
throw new SDKException("Invalid JWK in JWT header", e);
}
}

// Check for X.509 certificate chain in header
Comment on lines +419 to +428
Copy link
Member

Choose a reason for hiding this comment

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

good

if (verifier == null && header.getX509CertChain() != null && !header.getX509CertChain().isEmpty()) {
try {
X509Certificate cert = X509CertUtils.parse(header.getX509CertChain().get(0).decode());
if (cert.getPublicKey() instanceof RSAPublicKey) {
verifier = createVerifier((RSAPublicKey) cert.getPublicKey());
} else {
throw new SDKException("Unsupported public key type in X.509 certificate");
}
} catch (IllegalArgumentException e) {
throw new SDKException("Invalid Base64 in X.509 certificate in JWT header", e);
}
}
Comment on lines +428 to +440
Copy link
Contributor

Choose a reason for hiding this comment

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

critical

The current implementation for handling the x5c header in the JWT simply extracts the public key from the first certificate in the chain without performing any trust validation. This is a significant security vulnerability. An attacker could forge a JWT, sign it with their own key, and embed the corresponding public key within the x5c header. The system would then trust this key and validate the signature, effectively bypassing all trust checks.

The certificate chain provided in x5c should be validated against a configured trust store (containing trusted root or intermediate CAs) before its public key is used for signature verification.

I am not providing a code suggestion as a full implementation would require significant changes, including setting up a trust store. However, this vulnerability must be addressed. A possible approach would involve using java.security.cert.CertPathValidator.



if (verifier == null) {
verifier = createVerifier(assertionKey);
}


if (!signedJWT.verify(verifier)) {
throw new SDKException("Unable to verify assertion signature");
Expand All @@ -424,19 +458,27 @@ public Assertion.HashValues verify(AssertionConfig.AssertionKey assertionKey)

private SignedJWT createSignedJWT(final JWTClaimsSet claims, final AssertionConfig.AssertionKey assertionKey)
throws SDKException {
final JWSHeader jwsHeader;
final JWSHeader.Builder headerBuilder;
switch (assertionKey.alg) {
case RS256:
jwsHeader = new JWSHeader.Builder(JWSAlgorithm.RS256).build();
headerBuilder = new JWSHeader.Builder(JWSAlgorithm.RS256);
break;
case HS256:
jwsHeader = new JWSHeader.Builder(JWSAlgorithm.HS256).build();
headerBuilder = new JWSHeader.Builder(JWSAlgorithm.HS256);
break;
default:
throw new SDKException("Unknown assertion key algorithm, error signing assertion");
}

return new SignedJWT(jwsHeader, claims);
if (assertionKey.jwk != null) {
headerBuilder.jwk(assertionKey.jwk);
}

if (assertionKey.x5c != null) {
headerBuilder.x509CertChain(assertionKey.x5c);
}

return new SignedJWT(headerBuilder.build(), claims);
}

private JWSSigner createSigner(final AssertionConfig.AssertionKey assertionKey)
Expand All @@ -460,13 +502,30 @@ private JWSSigner createSigner(final AssertionConfig.AssertionKey assertionKey)
private JWSVerifier createVerifier(AssertionConfig.AssertionKey assertionKey) throws JOSEException {
switch (assertionKey.alg) {
case RS256:
return new RSASSAVerifier((RSAPublicKey) assertionKey.key);
if (assertionKey.key instanceof JWK) {
return createVerifier((JWK) assertionKey.key);
} else if (assertionKey.key instanceof RSAPublicKey) {
return createVerifier((RSAPublicKey) assertionKey.key);
} else {
throw new SDKException("Expected JWK or RSAPublicKey for RS256 algorithm");
}
case HS256:
return new MACVerifier((byte[]) assertionKey.key);
default:
throw new SDKException("Unknown verify key, unable to verify assertion signature");
}
}

private JWSVerifier createVerifier(JWK jwk) throws JOSEException {
if (jwk instanceof com.nimbusds.jose.jwk.RSAKey) {
return new RSASSAVerifier(jwk.toRSAKey());
}
throw new JOSEException("Unsupported JWK type: " + jwk.getKeyType() + ". Only RSA keys are supported.");
}

private JWSVerifier createVerifier(RSAPublicKey publicKey) {
return new RSASSAVerifier(publicKey);
}
}

public static class AssertionValueAdapter implements JsonDeserializer<AssertionConfig.Statement> {
Expand Down
2 changes: 1 addition & 1 deletion sdk/src/main/java/io/opentdf/platform/sdk/TDF.java
Original file line number Diff line number Diff line change
Expand Up @@ -695,7 +695,7 @@ Reader loadTDF(SeekableByteChannel tdf, Config.TDFReaderConfig tdfReaderConfig)
Manifest.Assertion.HashValues hashValues;
try {
hashValues = assertion.verify(assertionKey);
} catch (ParseException | JOSEException e) {
} catch (ParseException | JOSEException | java.security.cert.CertificateException e) {
throw new SDKException("error validating assertion hash", e);
}
var hashOfAssertionAsHex = assertion.hash();
Expand Down
Loading
Loading