| FazBrowse GitHub Viewer | Trending | | Home |
| Tools: [Download Repo ZIP] [Original HTTPS Page] |
1 parent 001a1ce commit e597b7a
7 files changed
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
@@ -603,7 +603,7 @@ | |||
| 603 | 603 | <li>mauromol <https://github.com/mauromol> - reporting that RSASSA-PSS could not be used with a RIPEMD digest through the JCA API, tracing it to the missing PSS registrations and to DigestFactory.isSameDigest treating two identical non-SHA digest names as different (issue #2381) Reporting that EDIPartyName encoded its DirectoryString members without the RFC 5280 sec. 4.2.1.6 context tags its own decoder requires, so the type could not parse its own output (issue #2380).</li> | |
| 604 | 604 | <li>jmeeder <https://github.com/jmeeder> - reporting that RFC 4998 evidence-record generation rejected time stamps from an authority naming the digest with NULL parameters where BC names it with them absent, both of which RFC 5754 requires a receiver to accept (issue #2379).</li> | |
| 605 | 605 | <li>rimuln <https://github.com/rimuln> - diagnosis and fix for PKCS12 getCertificateAlias returning the alias of an unrelated certificate, tracing it to the alias and certificate enumerations of the keystore's certs table diverging in order once keys() enumerated a copy (issue #2384, PR #2385).</li> | |
| 606 | - <li>Yu Bao <yubao@paypal.com> - reporting an API gap, on behalf of the PayPal Cyber Security Team, that the high-level OpenPGP message API (OpenPGPMessageProcessor / OpenPGPMessageInputStream) gave a caller no way to bound how far a compressed data packet expands, where the low-level PGPCompressedData it wraps has carried a bounded getDataStream(long) overload all along, and that OpenPGPPolicy exposed no equivalent property to set. Suggesting a protocol whitelist for CRL Distribution Point fetching, which is now the org.bouncycastle.x509.CRLDP_protocols property, and suggesting that an OCSP response was read up to the length the responder declared for itself, now capped by org.bouncycastle.ocsp.max_response_size.</li> | ||
| 606 | + <li>Yu Bao <yubao@paypal.com> - reporting an API gap, on behalf of the PayPal Cyber Security Team, that the high-level OpenPGP message API (OpenPGPMessageProcessor / OpenPGPMessageInputStream) gave a caller no way to bound how far a compressed data packet expands, where the low-level PGPCompressedData it wraps has carried a bounded getDataStream(long) overload all along, and that OpenPGPPolicy exposed no equivalent property to set. Suggesting a protocol whitelist for CRL Distribution Point fetching, which is now the org.bouncycastle.x509.CRLDP_protocols property, and suggesting that an OCSP response was read up to the length the responder declared for itself, now capped by org.bouncycastle.ocsp.max_response_size, and suggesting bounds on the OpenPGP ASCII armor headers, now capped by org.bouncycastle.openpgp.max_armor_header_length and org.bouncycastle.openpgp.max_armor_headers.</li> | ||
| 607 | 607 | </ul> | |
| 608 | 608 | </body> | |
| 609 | 609 | </html> | |
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
@@ -80,6 +80,25 @@ public class Properties | |||
| 80 | 80 | */ | |
| 81 | 81 | public static final String OCSP_MAX_RESPONSE_SIZE = "org.bouncycastle.ocsp.max_response_size"; | |
| 82 | 82 | ||
| 83 | + /** | ||
| 84 | + * The longest OpenPGP ASCII armor header line, in bytes, ArmoredInputStream will accumulate. | ||
| 85 | + * The headers are parsed when the stream is constructed, so this bounds what merely wrapping | ||
| 86 | + * an untrusted stream can allocate: a "header line" that never reaches a line terminator is | ||
| 87 | + * refused rather than read until the heap is gone. Default is 4096, far above the short | ||
| 88 | + * Version / Comment lines RFC 9580 sec. 6.2 describes; a value of zero or less is ignored and | ||
| 89 | + * the default used. | ||
| 90 | + */ | ||
| 91 | + public static final String OPENPGP_MAX_ARMOR_HEADER_LENGTH = "org.bouncycastle.openpgp.max_armor_header_length"; | ||
| 92 | + | ||
| 93 | + /** | ||
| 94 | + * The largest number of OpenPGP ASCII armor header lines ArmoredInputStream will accept before | ||
| 95 | + * the blank line that ends the header block. Bounds the companion case to | ||
| 96 | + * {@link #OPENPGP_MAX_ARMOR_HEADER_LENGTH}, where each line is short but they never stop | ||
| 97 | + * arriving. Default is 64, counting the armor header line itself; a value of zero or less is | ||
| 98 | + * ignored and the default used. | ||
| 99 | + */ | ||
| 100 | + public static final String OPENPGP_MAX_ARMOR_HEADERS = "org.bouncycastle.openpgp.max_armor_headers"; | ||
| 101 | + | ||
| 83 | 102 | /** | |
| 84 | 103 | * If set to "true", the BC PKCS#12 KeyStore will additionally accept (on load only) | |
| 85 | 104 | * SafeBags of type secretBag that use SunJCE's non-standard nested encoding — | |
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
@@ -31,6 +31,9 @@ public class Properties | |||
| 31 | 31 | public static final String X509_CRL_CACHE_TTL = "org.bouncycastle.x509.crl_cache_ttl"; | |
| 32 | 32 | public static final String X509_ENABLE_CRLDP = "org.bouncycastle.x509.enableCRLDP"; | |
| 33 | 33 | public static final String X509_CRLDP_PROTOCOLS = "org.bouncycastle.x509.CRLDP_protocols"; | |
| 34 | + public static final String OCSP_MAX_RESPONSE_SIZE = "org.bouncycastle.ocsp.max_response_size"; | ||
| 35 | + public static final String OPENPGP_MAX_ARMOR_HEADER_LENGTH = "org.bouncycastle.openpgp.max_armor_header_length"; | ||
| 36 | + public static final String OPENPGP_MAX_ARMOR_HEADERS = "org.bouncycastle.openpgp.max_armor_headers"; | ||
| 34 | 37 | public static final String PKCS12_ALLOW_SUN_SECRET_KEYS = "org.bouncycastle.pkcs12.allow_sun_secret_keys"; | |
| 35 | 38 | public static final String PKCS1_STRICT_DIGESTINFO = "org.bouncycastle.pkcs1.strict_digestinfo"; | |
| 36 | 39 | public static final String JSSE_HOSTNAME_CHECK_CN_FALLBACK = "org.bouncycastle.jsse.hostname_check_cn_fallback"; | |
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
@@ -113,6 +113,7 @@ <h3>2.1.2 Defects Fixed</h3> | |||
| 113 | 113 | <li>The CRL fetcher behind X509RevocationChecker still downcast the opened connection to HttpURLConnection, so a CRL Distribution Point naming any protocol other than http or https threw a ClassCastException out of a method that declares IOException / CRLException. That downcast was removed from the provider's copy of CrlCache in the 1.68 cycle, because it stopped FTP based CRLs loading (github #1867), but the pkix copy was left as it was and has carried the defect since; its only caller catches Exception around the fetch, so the effect was not a visible failure but a distribution point quietly logged as ignored. The two fetchers are now identical. The same downcast has also been removed from the jdk1.4 overlay of the provider's CrlCache, which had likewise never had the #1867 fix applied - note that overlay still cannot set a connect or read timeout on the connection, as URLConnection.setConnectTimeout and setReadTimeout are Java 5.</li> | |
| 114 | 114 | <li>X509RevocationChecker downloaded CRLs from a certificate's CRL Distribution Points extension whenever the CRLs it had been given could not answer for a certificate, with no way for a caller to prevent it - unlike the provider's CertPath validator, which has always required the org.bouncycastle.x509.enableCRLDP property to be set, and unlike what that property's own javadoc describes ("the BC CertPath validator and X509RevocationChecker will attempt to download CRLs ..."). The checker now honours the property, so the URI taken from a certificate is only dereferenced when a caller has asked for that. <b>This is a behavioural change for anyone relying on the previous automatic fetch</b>: with the property unset the checker now behaves exactly as an unproductive fetch always did, soft failing where setSoftFail was configured and reporting no CRL found where it was not, so such callers should set org.bouncycastle.x509.enableCRLDP to "true" to keep the old behaviour.</li> | |
| 115 | 115 | <li>OcspCache.getOcspResponse read an OCSP response up to the length the responder itself declared in its Content-Length header, applying its own 32K ceiling only when that header was absent, so a responder (or anything able to answer in its place) that declared and sent hundreds of megabytes was read into the caller's heap. The declared length may now only narrow the read, never widen it: it is used when present and no larger than the ceiling, which is otherwise applied. The ceiling is now configurable through the new org.bouncycastle.ocsp.max_response_size property and its default has been raised from 32K to 64K, still far above any real response; a configured value of zero or less is ignored so a mistyped value cannot turn the limit off. An over-long response fails the OCSP check the same way an unreachable responder does - a caller with CRLs configured falls back to those - and now says so, as Streams.readAllLimited reports the condition only as "Data Overflow", which is restated in terms of the limit and the property that sets it.</li> | |
| 116 | + <li>ArmoredInputStream parses the OpenPGP ASCII armor headers as the stream is constructed, and bounded neither the length of a header line nor the number of them, so merely wrapping an untrusted stream could exhaust the heap before the caller had read a byte. Two shapes reached it: a "header line" that never arrives at a line terminator, accumulated without limit, and short header lines that never stop arriving, added to the header list without limit - the second is the companion case, reachable even where every line is well formed. Both are now capped, at 4096 bytes per header line and 64 headers (counting the armor header line itself), each configurable through the new org.bouncycastle.openpgp.max_armor_header_length and org.bouncycastle.openpgp.max_armor_headers properties; a value of zero or less is ignored, so a mistyped value cannot turn a limit off. Exceeding either raises ArmoredInputException, as the other malformed-header cases in that parser do. RFC 9580 sec. 6.2 bounds neither, but the headers it describes - Version, Comment and the like - are short, so the defaults sit far above any real armor. The equivalent accumulation in org.bouncycastle.util.io.pem.PemReader is deliberately left unbounded: a PEM body can legitimately be enormous - a CRL in the gigabyte range - so a default cap there would break working deployments.</li> | ||
| 116 | 117 | </ul> | |
| 117 | 118 | ||
| 118 | 119 | <h3>2.1.3 Additional Features and Functionality</h3> | |
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
@@ -9,6 +9,7 @@ | |||
| 9 | 9 | import java.util.List; | |
| 10 | 10 | ||
| 11 | 11 | import org.bouncycastle.util.StringList; | |
| 12 | + import org.bouncycastle.util.Properties; | ||
| 12 | 13 | import org.bouncycastle.util.Strings; | |
| 13 | 14 | ||
| 14 | 15 | /** | |
@@ -268,6 +269,20 @@ public int available() | |||
| 268 | 269 | return in.available(); | |
| 269 | 270 | } | |
| 270 | 271 | ||
| 272 | + private static final int DEFAULT_MAX_ARMOR_HEADER_LENGTH = 4096; | ||
| 273 | + private static final int DEFAULT_MAX_ARMOR_HEADERS = 64; | ||
| 274 | + | ||
| 275 | + /** | ||
| 276 | + * A configured limit that cannot be one leaves the default in place, so a mistyped value | ||
| 277 | + * cannot turn the limit off. | ||
| 278 | + */ | ||
| 279 | + private static int armorLimit(String propertyName, int defaultValue) | ||
| 280 | + { | ||
| 281 | + int value = Properties.asInteger(propertyName, defaultValue); | ||
| 282 | + | ||
| 283 | + return value > 0 ? value : defaultValue; | ||
| 284 | + } | ||
| 285 | + | ||
| 271 | 286 | private boolean parseHeaders() | |
| 272 | 287 | throws IOException | |
| 273 | 288 | { | |
@@ -305,6 +320,15 @@ private boolean parseHeaders() | |||
| 305 | 320 | boolean eolReached = false; | |
| 306 | 321 | boolean crLf = false; | |
| 307 | 322 | ||
| 323 | + /* | ||
| 324 | + * The headers are parsed as the stream is constructed, so these bound what wrapping an | ||
| 325 | + * untrusted stream can allocate before the caller has read a byte: a "header line" that | ||
| 326 | + * never reaches a terminator, and header lines that never stop arriving. | ||
| 327 | + */ | ||
| 328 | + int maxHeaderLength = armorLimit(Properties.OPENPGP_MAX_ARMOR_HEADER_LENGTH, | ||
| 329 | + DEFAULT_MAX_ARMOR_HEADER_LENGTH); | ||
| 330 | + int maxHeaders = armorLimit(Properties.OPENPGP_MAX_ARMOR_HEADERS, DEFAULT_MAX_ARMOR_HEADERS); | ||
| 331 | + | ||
| 308 | 332 | ByteArrayOutputStream buf = new ByteArrayOutputStream(); | |
| 309 | 333 | buf.write('-'); | |
| 310 | 334 | ||
@@ -347,12 +371,24 @@ private boolean parseHeaders() | |||
| 347 | 371 | { | |
| 348 | 372 | throw new ArmoredInputException("invalid armor header"); | |
| 349 | 373 | } | |
| 374 | + if (headerList.size() >= maxHeaders) | ||
| 375 | + { | ||
| 376 | + throw new ArmoredInputException("more than " + maxHeaders + " armor headers (see " | ||
| 377 | + + Properties.OPENPGP_MAX_ARMOR_HEADERS + ")"); | ||
| 378 | + } | ||
| 379 | + | ||
| 350 | 380 | headerList.add(line); | |
| 351 | 381 | buf.reset(); | |
| 352 | 382 | } | |
| 353 | 383 | ||
| 354 | 384 | if (c != '\n' && c != '\r') | |
| 355 | 385 | { | |
| 386 | + if (buf.size() >= maxHeaderLength) | ||
| 387 | + { | ||
| 388 | + throw new ArmoredInputException("armor header line exceeds " + maxHeaderLength | ||
| 389 | + + " bytes (see " + Properties.OPENPGP_MAX_ARMOR_HEADER_LENGTH + ")"); | ||
| 390 | + } | ||
| 391 | + | ||
| 356 | 392 | buf.write(c); | |
| 357 | 393 | eolReached = false; | |
| 358 | 394 | } | |
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
@@ -30,6 +30,7 @@ public void testPacketParsing() | |||
| 30 | 30 | new TimeEncodingTest(), | |
| 31 | 31 | new ArmoredOutputStreamHeaderInjectionTest(), | |
| 32 | 32 | new ArmoredInputStreamCrcTest(), | |
| 33 | + new ArmoredInputStreamHeaderLimitTest(), | ||
| 33 | 34 | new UserAttributeSubpacketInputStreamTest(), | |
| 34 | 35 | new UnknownPublicKeyPacketTest(), | |
| 35 | 36 | new UnknownSecretKeyPacketTest(), | |
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
@@ -0,0 +1,153 @@ | |||
| 1 | + package org.bouncycastle.bcpg.test; | ||
| 2 | + | ||
| 3 | + import java.io.ByteArrayInputStream; | ||
| 4 | + import java.io.ByteArrayOutputStream; | ||
| 5 | + import java.io.IOException; | ||
| 6 | + | ||
| 7 | + import org.bouncycastle.bcpg.ArmoredInputStream; | ||
| 8 | + import org.bouncycastle.bcpg.ArmoredOutputStream; | ||
| 9 | + import org.bouncycastle.util.Arrays; | ||
| 10 | + import org.bouncycastle.util.Properties; | ||
| 11 | + import org.bouncycastle.util.Strings; | ||
| 12 | + import org.bouncycastle.util.io.Streams; | ||
| 13 | + | ||
| 14 | + /** | ||
| 15 | + * ArmoredInputStream parses the armor headers as it is constructed, so what it accumulates there is | ||
| 16 | + * reachable by merely wrapping an untrusted stream. Neither the length of a header line nor the | ||
| 17 | + * number of them was bounded: a "header line" that never reaches a terminator, and short header | ||
| 18 | + * lines that never stop arriving, each exhausted the heap inside the constructor. Both are now | ||
| 19 | + * capped, by org.bouncycastle.openpgp.max_armor_header_length and | ||
| 20 | + * org.bouncycastle.openpgp.max_armor_headers. | ||
| 21 | + */ | ||
| 22 | + public class ArmoredInputStreamHeaderLimitTest | ||
| 23 | + extends AbstractPacketTest | ||
| 24 | + { | ||
| 25 | + public String getName() | ||
| 26 | + { | ||
| 27 | + return "ArmoredInputStreamHeaderLimitTest"; | ||
| 28 | + } | ||
| 29 | + | ||
| 30 | + public void performTest() | ||
| 31 | + throws Exception | ||
| 32 | + { | ||
| 33 | + overLongHeaderLineIsRejected(); | ||
| 34 | + tooManyHeadersAreRejected(); | ||
| 35 | + configuredLimitsAreHonoured(); | ||
| 36 | + ordinaryArmorStillRoundTrips(); | ||
| 37 | + } | ||
| 38 | + | ||
| 39 | + private void overLongHeaderLineIsRejected() | ||
| 40 | + throws IOException | ||
| 41 | + { | ||
| 42 | + StringBuffer armor = new StringBuffer("-----BEGIN PGP MESSAGE-----\nComment: "); | ||
| 43 | + for (int i = 0; i != 5000; i++) | ||
| 44 | + { | ||
| 45 | + armor.append('A'); | ||
| 46 | + } | ||
| 47 | + armor.append("\n\n"); | ||
| 48 | + | ||
| 49 | + try | ||
| 50 | + { | ||
| 51 | + new ArmoredInputStream(new ByteArrayInputStream(Strings.toByteArray(armor.toString()))); | ||
| 52 | + fail("over-long armor header line accepted"); | ||
| 53 | + } | ||
| 54 | + catch (IOException e) | ||
| 55 | + { | ||
| 56 | + isEquals("armor header line exceeds 4096 bytes (see org.bouncycastle.openpgp.max_armor_header_length)", | ||
| 57 | + e.getMessage()); | ||
| 58 | + } | ||
| 59 | + } | ||
| 60 | + | ||
| 61 | + private void tooManyHeadersAreRejected() | ||
| 62 | + throws IOException | ||
| 63 | + { | ||
| 64 | + StringBuffer armor = new StringBuffer("-----BEGIN PGP MESSAGE-----\n"); | ||
| 65 | + for (int i = 0; i != 100; i++) | ||
| 66 | + { | ||
| 67 | + armor.append("Comment: x\n"); | ||
| 68 | + } | ||
| 69 | + armor.append("\n"); | ||
| 70 | + | ||
| 71 | + try | ||
| 72 | + { | ||
| 73 | + new ArmoredInputStream(new ByteArrayInputStream(Strings.toByteArray(armor.toString()))); | ||
| 74 | + fail("unbounded run of armor headers accepted"); | ||
| 75 | + } | ||
| 76 | + catch (IOException e) | ||
| 77 | + { | ||
| 78 | + isEquals("more than 64 armor headers (see org.bouncycastle.openpgp.max_armor_headers)", e.getMessage()); | ||
| 79 | + } | ||
| 80 | + } | ||
| 81 | + | ||
| 82 | + /** | ||
| 83 | + * The limits are configurable in both directions, and a value that cannot be a limit leaves the | ||
| 84 | + * default in place rather than removing it. | ||
| 85 | + */ | ||
| 86 | + private void configuredLimitsAreHonoured() | ||
| 87 | + throws IOException | ||
| 88 | + { | ||
| 89 | + StringBuffer armor = new StringBuffer("-----BEGIN PGP MESSAGE-----\n"); | ||
| 90 | + for (int i = 0; i != 20; i++) | ||
| 91 | + { | ||
| 92 | + armor.append("Comment: x\n"); | ||
| 93 | + } | ||
| 94 | + armor.append("\n"); | ||
| 95 | + | ||
| 96 | + byte[] armored = Strings.toByteArray(armor.toString()); | ||
| 97 | + | ||
| 98 | + // within the default limit of 64 | ||
| 99 | + new ArmoredInputStream(new ByteArrayInputStream(armored)); | ||
| 100 | + | ||
| 101 | + System.setProperty(Properties.OPENPGP_MAX_ARMOR_HEADERS, "8"); | ||
| 102 | + try | ||
| 103 | + { | ||
| 104 | + new ArmoredInputStream(new ByteArrayInputStream(armored)); | ||
| 105 | + fail("configured armor header limit ignored"); | ||
| 106 | + } | ||
| 107 | + catch (IOException e) | ||
| 108 | + { | ||
| 109 | + isEquals("more than 8 armor headers (see org.bouncycastle.openpgp.max_armor_headers)", e.getMessage()); | ||
| 110 | + } | ||
| 111 | + finally | ||
| 112 | + { | ||
| 113 | + System.getProperties().remove(Properties.OPENPGP_MAX_ARMOR_HEADERS); | ||
| 114 | + } | ||
| 115 | + | ||
| 116 | + System.setProperty(Properties.OPENPGP_MAX_ARMOR_HEADERS, "0"); | ||
| 117 | + try | ||
| 118 | + { | ||
| 119 | + // "0" cannot be a limit, so the default applies and this is accepted | ||
| 120 | + new ArmoredInputStream(new ByteArrayInputStream(armored)); | ||
| 121 | + } | ||
| 122 | + finally | ||
| 123 | + { | ||
| 124 | + System.getProperties().remove(Properties.OPENPGP_MAX_ARMOR_HEADERS); | ||
| 125 | + } | ||
| 126 | + } | ||
| 127 | + | ||
| 128 | + private void ordinaryArmorStillRoundTrips() | ||
| 129 | + throws IOException | ||
| 130 | + { | ||
| 131 | + byte[] data = Strings.toByteArray("Hello, World!"); | ||
| 132 | + | ||
| 133 | + ByteArrayOutputStream bOut = new ByteArrayOutputStream(); | ||
| 134 | + ArmoredOutputStream aOut = ArmoredOutputStream.builder() | ||
| 135 | + .setVersion("BCPG") | ||
| 136 | + .addComment("a comment") | ||
| 137 | + .build(bOut); | ||
| 138 | + | ||
| 139 | + aOut.write(data); | ||
| 140 | + aOut.close(); | ||
| 141 | + | ||
| 142 | + ArmoredInputStream aIn = new ArmoredInputStream(new ByteArrayInputStream(bOut.toByteArray())); | ||
| 143 | + | ||
| 144 | + isTrue("armored round trip", Arrays.areEqual(data, Streams.readAll(aIn))); | ||
| 145 | + | ||
| 146 | + aIn.close(); | ||
| 147 | + } | ||
| 148 | + | ||
| 149 | + public static void main(String[] args) | ||
| 150 | + { | ||
| 151 | + runTest(new ArmoredInputStreamHeaderLimitTest()); | ||
| 152 | + } | ||
| 153 | + } | ||
| Back | FazBrowse Home | New Git URL |
0 commit comments