Repository: netty/netty
Issue: SSL certificate parsing issue when upgrading beyond 4.1.78 on Android

### Expected behavior

We're running a gRPC server on Android. We use SSL and have certificates that work until version 4.1.79, and we'd like to be able to continue to use them on more recent versions. Looking for hints to debug why this would suddenly start failing.

### Actual behavior

Certificates fail to parse with:

```
org.conscrypt.OpenSSLX509CertificateFactory$ParsingException: org.conscrypt.OpenSSLX509CertificateFactory$ParsingException: java.lang.RuntimeException: error:0c0000a2:ASN.1 encoding routines:OPENSSL_internal:NOT_ENOUGH_DATA
```

### Steps to reproduce

Upgrade Netty.

### Minimal yet complete reproducer code (or URL to code)

Unfortunately this is a complex and proprietary project so it's difficult to provide a reproduction. The relevant interactions are between Netty and gRPC version 1.47.0 and conscrypt-android version 2.5.1, and the code is running on Android API 28.

If the short diff between 4.1.78 and 4.1.79 is not enough to find a good solution I can try to build a minimum reproducer. I've also tried to use Jitpack to build intermediate commits but I'm having trouble getting those to run on Android.

### Netty version

4.1.79

### JVM version (e.g. `java -version`)

Android API 28.

### OS version (e.g. `uname -a`)

Android 9.

Issue comments:
Via local builds I was able to isolate the problematic commit: https://github.com/netty/netty/commit/dc69a943c3682fec16845828cb6d8f0554fad61f

Still not sure what this breaks - we're using standard PEM format like:

```
-----BEGIN CERTIFICATE-----
<a bunch of base 64>
-----END CERTIFICATE-----
```

with none of the newlines this commit seems to address.
@andrewparmet I guess you can't share the pem file ?
Unfortunately not. I've been able to narrow it down a bit:

- Parsing does work on the JVM - this is only an Android issue
- The text returned by [`m.group(0)` here](https://github.com/netty/netty/blob/aa59245955f5121c4230d46c41a4b7f55e0c9ba7/handler/src/main/java/io/netty/handler/ssl/PemReader.java#L92) is simply `BEGIN` - which is not right at all. I'd have a hard time believing it, but is there a difference between the regex implementations on the JVM and Android?
hmm... sounds like an android bug then... 
Sure does. Here's a minimal reproducer:

```kotlin
private val CERT_HEADER = Pattern.compile("-+BEGIN\\s[^-\\r\\n]*CERTIFICATE[^-\\r\\n]*-+(?:\\s|\\r|\\n)+")
private val BODY = Pattern.compile("[a-z0-9+/=][a-z0-9+/=\\r\\n]*", Pattern.CASE_INSENSITIVE)

class RegexTest {
    @Test
    fun regexTest() {
        val content =
            """
                |-----BEGIN CERTIFICATE-----
                |abcdef
                |-----END CERTIFICATE-----
            """.trimMargin()

        val m = CERT_HEADER.matcher(content)
        assertTrue(m.find(0))
        m.usePattern(BODY)
        assertTrue(m.find())
        val body = m.group(0)
        assertEquals("abcdef\n", body)
    }
}
```

This test passes on the JVM (OpenJDK Runtime Environment Corretto-11.0.18.10.1 (build 11.0.18+10-LTS)) and fails on Android 34.
Filed: https://issuetracker.google.com/issues/293206296 and we'll see what comes back.
@andrewparmet thanks... keep us posted 
Hi there,
I have the same issue here. 

I did some research and found that this is probably not an Android issue.
PemReader class uses regex to parse the certificates from the InputStream, but the thing is that regex works differently in JVM and Android Runtime.
https://github.com/netty/netty/blob/285ba74f899866ddf3e23a6770c954e350245bd7/handler/src/main/java/io/netty/handler/ssl/PemReader.java#L84C1-L90C14
PemReader expects that after matching the header the matcher moves the cursor to a position after the header and it works this way in JVM but it doesn't in Android and the certificate body is matched starting from the beginning of the string as just "BEGIN". And this is why the conversion to an X509 certificate instance fails.

This can be fixed by calling the find() method with the next position after the header as a parameter, or by using the BouncyCastle PEMParser as it's done for the private key.

@fishb1 if you know how to fix it please open a PR and we will review it 
You first say:

> this is probably not an Android issue

Then you say:

> regex works differently in JVM and Android Runtime

To me this seems like the definition of "Android issue" but if you have a workaround that fixes the implementation on Android while not breaking it on the JVM, then to me that seems like only mild debt (when paired with comment to undo the hack and a link to the Android bug that I filed and Google accepted).

In any case we've migrated to using binary certificate files instead of text so we're not held hostage by this bug anymore.
> To me this seems like the definition of "Android issue" but if you have a workaround that fixes the implementation on Android while not breaking it on the JVM, then to me that seems like only mild debt (when paired with comment to undo the hack and a link to the Android bug that I filed and Google accepted).
> 

Ahhh, I see now! I expected that usePatten() method reset the latest match position but I read the doc for the method again and it said that the position should not be affected. 

Thanks for submitting this! 

But I think a fix needs to be done anyway to make Netty work in older SDKs, even if Google fixes the regex issue in future versions.

Pull request review comments:

Patch that resolved the issue:
diff --git a/handler/src/main/java/io/netty/handler/ssl/PemReader.java b/handler/src/main/java/io/netty/handler/ssl/PemReader.java
index 0ee79b71457..33328ea672d 100644
--- a/handler/src/main/java/io/netty/handler/ssl/PemReader.java
+++ b/handler/src/main/java/io/netty/handler/ssl/PemReader.java
@@ -84,14 +84,21 @@ static ByteBuf[] readCertificates(InputStream in) throws CertificateException {
             if (!m.find(start)) {
                 break;
             }
+
+            // Here and below it's necessary to save the position as it is reset
+            // after calling usePattern() on Android due to a bug.
+            //
+            // See https://issuetracker.google.com/issues/293206296
+            start = m.end();
             m.usePattern(BODY);
-            if (!m.find()) {
+            if (!m.find(start)) {
                 break;
             }
 
             ByteBuf base64 = Unpooled.copiedBuffer(m.group(0), CharsetUtil.US_ASCII);
+            start = m.end();
             m.usePattern(CERT_FOOTER);
-            if (!m.find()) {
+            if (!m.find(start)) {
                 // Certificate is incomplete.
                 break;
             }
@@ -131,19 +138,21 @@ static ByteBuf readPrivateKey(InputStream in) throws KeyException {
         } catch (IOException e) {
             throw new KeyException("failed to read key input stream", e);
         }
-
+        int start = 0;
         Matcher m = KEY_HEADER.matcher(content);
-        if (!m.find()) {
+        if (!m.find(start)) {
             throw keyNotFoundException();
         }
+        start = m.end();
         m.usePattern(BODY);
-        if (!m.find()) {
+        if (!m.find(start)) {
             throw keyNotFoundException();
         }
 
         ByteBuf base64 = Unpooled.copiedBuffer(m.group(0), CharsetUtil.US_ASCII);
+        start = m.end();
         m.usePattern(KEY_FOOTER);
-        if (!m.find()) {
+        if (!m.find(start)) {
             // Key is incomplete.
             throw keyNotFoundException();
         }


Test patch: