Fix/xxe cot xml parsing#73
Open
1456055067 wants to merge 3 commits into
Open
Conversation
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).
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Two independent hardening fixes found during a security review:
XXE on CoT / mission XML parsing. dom4j's
new SAXReader()andDocumentHelper.parseText()resolve DOCTYPE declarations and external entities bydefault. 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.
Predictable certificate serial numbers. The CA signing path set the X.509 serial
from
new java.util.Random().nextInt() & 0x7fffffff— a non-cryptographic, seedablePRNG masked to 31 bits. Predictable / collision-prone serials weaken hash-collision
forgery defenses and can break serial-keyed revocation (CRL/OCSP).
Changes
XXE
SecureSaxReader(dom4j counterpart of the existingSecureXmlParser) intakserver-plugins, visible to core/war/retention via theapidependency. DisablesDOCTYPE declarations + external general/parameter entities + external DTD loading,
enables secure processing, and fails closed if hardening can't be applied.
StreamingProtoBufHelper.proto2cot(the
xmlDetailof every inbound protobuf CoT — the default client protocol),ProtoBufHelper(federated detail), mission-package manifests(
DataPackageFileBlocker,MissionPackageExtractor, retentionMissionArchiveHelper),and stored-detail/protocol/injector sites (
CotImageBean,KmlUtils,KMLServiceImpl,TracksKMLServlet,MissionServiceDefaultImpl,StreamingProtoBuf[OrCoT]Protocol,DistributedSubmissionService,DistributedSubscriptionManager,UidCotTagInjector).Cert serials
CertManagernow uses a sharedSecureRandomand a 64-bit serial(
new BigInteger(64, serialRandom); CA/Browser Forum BR 7.1 minimum).Compatibility / deployment notes
XML never contains a DOCTYPE. Any input that does declare a DOCTYPE is now rejected.
Testing
:takserver-core:takserver-war:compileJava✅:takserver-retention:compileJava✅:takserver-core:compileTestJava✅Follow-ups (not in this PR)
signatureAlgis accepted without rejecting weak algorithms(MD5/SHA1) — add an allowlist.