FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

Bound the length and the number of OpenPGP ASCII armor header lines A… · bcgit/bc-java@e597b7a · GitHub

Repository navigation

Commit e597b7a

Browse files
committed
Bound the length and the number of OpenPGP ASCII armor header lines ArmoredInputStream will accumulate while parsing them at construction, with both limits set by org.bouncycastle.openpgp.max_armor_header_length and org.bouncycastle.openpgp.max_armor_headers.
1 parent 001a1ce commit e597b7a

7 files changed

Lines changed: 214 additions & 1 deletion

File tree

‎CONTRIBUTORS.html‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -603,7 +603,7 @@
603603
<li>mauromol &lt;https://github.com/mauromol&gt; - 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>
604604
<li>jmeeder &lt;https://github.com/jmeeder&gt; - 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>
605605
<li>rimuln &lt;https://github.com/rimuln&gt; - 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 &lt;yubao@paypal.com&gt; - 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 &lt;yubao@paypal.com&gt; - 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>
607607
</ul>
608608
</body>
609609
</html>

‎core/src/main/java/org/bouncycastle/util/Properties.java‎

Lines changed: 19 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -80,6 +80,25 @@ public class Properties
8080
*/
8181
public static final String OCSP_MAX_RESPONSE_SIZE = "org.bouncycastle.ocsp.max_response_size";
8282

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+
83102
/**
84103
* If set to "true", the BC PKCS#12 KeyStore will additionally accept (on load only)
85104
* SafeBags of type secretBag that use SunJCE's non-standard nested encoding —

‎core/src/main/jdk1.4/org/bouncycastle/util/Properties.java‎

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -31,6 +31,9 @@ public class Properties
3131
public static final String X509_CRL_CACHE_TTL = "org.bouncycastle.x509.crl_cache_ttl";
3232
public static final String X509_ENABLE_CRLDP = "org.bouncycastle.x509.enableCRLDP";
3333
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";
3437
public static final String PKCS12_ALLOW_SUN_SECRET_KEYS = "org.bouncycastle.pkcs12.allow_sun_secret_keys";
3538
public static final String PKCS1_STRICT_DIGESTINFO = "org.bouncycastle.pkcs1.strict_digestinfo";
3639
public static final String JSSE_HOSTNAME_CHECK_CN_FALLBACK = "org.bouncycastle.jsse.hostname_check_cn_fallback";

‎docs/releasenotes.html‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -113,6 +113,7 @@ <h3>2.1.2 Defects Fixed</h3>
113113
<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>
114114
<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>
115115
<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>
116117
</ul>
117118

118119
<h3>2.1.3 Additional Features and Functionality</h3>

‎pg/src/main/java/org/bouncycastle/bcpg/ArmoredInputStream.java‎

Lines changed: 36 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -9,6 +9,7 @@
99
import java.util.List;
1010

1111
import org.bouncycastle.util.StringList;
12+
import org.bouncycastle.util.Properties;
1213
import org.bouncycastle.util.Strings;
1314

1415
/**
@@ -268,6 +269,20 @@ public int available()
268269
return in.available();
269270
}
270271

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+
271286
private boolean parseHeaders()
272287
throws IOException
273288
{
@@ -305,6 +320,15 @@ private boolean parseHeaders()
305320
boolean eolReached = false;
306321
boolean crLf = false;
307322

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+
308332
ByteArrayOutputStream buf = new ByteArrayOutputStream();
309333
buf.write('-');
310334

@@ -347,12 +371,24 @@ private boolean parseHeaders()
347371
{
348372
throw new ArmoredInputException("invalid armor header");
349373
}
374+
if (headerList.size() >= maxHeaders)
375+
{
376+
throw new ArmoredInputException("more than " + maxHeaders + " armor headers (see "
377+
+ Properties.OPENPGP_MAX_ARMOR_HEADERS + ")");
378+
}
379+
350380
headerList.add(line);
351381
buf.reset();
352382
}
353383

354384
if (c != '\n' && c != '\r')
355385
{
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+
356392
buf.write(c);
357393
eolReached = false;
358394
}

‎pg/src/test/java/org/bouncycastle/bcpg/test/AllTests.java‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -30,6 +30,7 @@ public void testPacketParsing()
3030
new TimeEncodingTest(),
3131
new ArmoredOutputStreamHeaderInjectionTest(),
3232
new ArmoredInputStreamCrcTest(),
33+
new ArmoredInputStreamHeaderLimitTest(),
3334
new UserAttributeSubpacketInputStreamTest(),
3435
new UnknownPublicKeyPacketTest(),
3536
new UnknownSecretKeyPacketTest(),
Lines changed: 153 additions & 0 deletions
Original file line numberDiff line numberDiff 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+
}

0 commit comments

Comments
 (0)

Back | FazBrowse Home | New Git URL