Skip to content

Fix/xxe cot xml parsing#73

Open
1456055067 wants to merge 3 commits into
TAK-Product-Center:mainfrom
1456055067:fix/xxe-cot-xml-parsing
Open

Fix/xxe cot xml parsing#73
1456055067 wants to merge 3 commits into
TAK-Product-Center:mainfrom
1456055067:fix/xxe-cot-xml-parsing

Conversation

@1456055067

Copy link
Copy Markdown

Summary

Two independent hardening fixes found during a security review:

  1. XXE on CoT / mission XML parsing. dom4j's new SAXReader() and
    DocumentHelper.parseText() resolve DOCTYPE declarations and external entities by
    default. Several parse sites consumed XML from untrusted sources (inbound protobuf CoT,
    federated detail, client-uploaded mission packages) without hardening, allowing XXE
    (local file disclosure, SSRF) and entity-expansion DoS.

  2. Predictable certificate serial numbers. The CA signing path set the X.509 serial
    from new java.util.Random().nextInt() & 0x7fffffff — a non-cryptographic, seedable
    PRNG masked to 31 bits. Predictable / collision-prone serials weaken hash-collision
    forgery defenses and can break serial-keyed revocation (CRL/OCSP).

Changes

XXE

  • Add SecureSaxReader (dom4j counterpart of the existing SecureXmlParser) in
    takserver-plugins, visible to core/war/retention via the api dependency. Disables
    DOCTYPE declarations + external general/parameter entities + external DTD loading,
    enables secure processing, and fails closed if hardening can't be applied.
  • Route all untrusted parse sites through it: StreamingProtoBufHelper.proto2cot
    (the xmlDetail of every inbound protobuf CoT — the default client protocol),
    ProtoBufHelper (federated detail), mission-package manifests
    (DataPackageFileBlocker, MissionPackageExtractor, retention MissionArchiveHelper),
    and stored-detail/protocol/injector sites (CotImageBean, KmlUtils, KMLServiceImpl,
    TracksKMLServlet, MissionServiceDefaultImpl, StreamingProtoBuf[OrCoT]Protocol,
    DistributedSubmissionService, DistributedSubscriptionManager, UidCotTagInjector).
  • Hardcoded-literal parse calls (server-generated constants) left unchanged.

Cert serials

  • CertManager now uses a shared SecureRandom and a 64-bit serial
    (new BigInteger(64, serialRandom); CA/Browser Forum BR 7.1 minimum).

Compatibility / deployment notes

  • XXE: no behavioral change for valid traffic — legitimate CoT / KML / mission-manifest
    XML never contains a DOCTYPE. Any input that does declare a DOCTYPE is now rejected.
  • Cert serials: affects only newly issued certificates; existing certs are unaffected.

Testing

  • :takserver-core:takserver-war:compileJava
  • :takserver-retention:compileJava
  • :takserver-core:compileTestJava
  • Integration tests not run in this environment (require DB/Ignite).

Follow-ups (not in this PR)

  • Configurable CA signatureAlg is accepted without rejecting weak algorithms
    (MD5/SHA1) — add an allowlist.
  • Several TLS clients disable cert/hostname verification (OAuth/Keycloak/federation).

dom4j SAXReader and DocumentHelper.parseText resolve DOCTYPE declarations and
external entities by default. Several parse sites consumed XML from untrusted
sources without hardening, allowing XXE (local file disclosure, SSRF) and
entity-expansion DoS:

- StreamingProtoBufHelper.proto2cot parsed the free-form xmlDetail of every
  inbound protobuf CoT message (the default protocol for current clients).
- ProtoBufHelper parsed federated detail XML from peer servers.
- Mission-package manifest parsing (DataPackageFileBlocker, MissionPackageExtractor,
  retention MissionArchiveHelper) parsed client-uploaded/synced packages.
- Various stored-CoT-detail re-parse sites (KML/track/image/mission services).

Add SecureSaxReader (dom4j counterpart of SecureXmlParser) which disables
DOCTYPE declarations and external entities, and route all untrusted parse
sites through it. Hardcoded-literal parse calls are left unchanged.
CertManager.signCertificate (the production CA signing path for client and
server certs) set the X.509 serial from `new java.util.Random().nextInt() &
0x7fffffff` — a non-cryptographic, seedable PRNG masked to 31 bits. Predictable
or collision-prone serials weaken hash-collision forgery defenses and can break
serial-keyed revocation (CRL/OCSP).

Use a shared SecureRandom and a 64-bit serial (CA/Browser Forum BR 7.1 minimum).
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant