From bdc968dd430712c80ccbe8aacdbb4411a8a8b9e2 Mon Sep 17 00:00:00 2001 From: Paul Schaub Date: Sat, 8 Oct 2022 13:57:53 +0200 Subject: [PATCH] Create TeeBCPGInputStream to move teeing logic out of OpenPgpMessageInputStream --- .../DelayedTeeInputStreamInputStream.java | 38 ----- .../OpenPgpMessageInputStream.java | 107 +++++--------- .../TeeBCPGInputStream.java | 131 ++++++++++++++++++ .../automaton/InputAlphabet.java | 4 +- .../automaton/PDA.java | 18 +-- .../OpenPgpMessageInputStreamTest.java | 3 +- .../automaton/PDATest.java | 10 +- 7 files changed, 182 insertions(+), 129 deletions(-) delete mode 100644 pgpainless-core/src/main/java/org/pgpainless/decryption_verification/DelayedTeeInputStreamInputStream.java create mode 100644 pgpainless-core/src/main/java/org/pgpainless/decryption_verification/TeeBCPGInputStream.java diff --git a/pgpainless-core/src/main/java/org/pgpainless/decryption_verification/DelayedTeeInputStreamInputStream.java b/pgpainless-core/src/main/java/org/pgpainless/decryption_verification/DelayedTeeInputStreamInputStream.java deleted file mode 100644 index 5cbabf89..00000000 --- a/pgpainless-core/src/main/java/org/pgpainless/decryption_verification/DelayedTeeInputStreamInputStream.java +++ /dev/null @@ -1,38 +0,0 @@ -package org.pgpainless.decryption_verification; - -import java.io.IOException; -import java.io.InputStream; -import java.io.OutputStream; - -public class DelayedTeeInputStreamInputStream extends InputStream { - - private int last = -1; - private final InputStream inputStream; - private final OutputStream outputStream; - - public DelayedTeeInputStreamInputStream(InputStream inputStream, OutputStream outputStream) { - this.inputStream = inputStream; - this.outputStream = outputStream; - } - - @Override - public int read() throws IOException { - if (last != -1) { - outputStream.write(last); - } - last = inputStream.read(); - return last; - } - - /** - * Squeeze the last byte out and update the output stream. - * - * @throws IOException in case of an IO error - */ - public void squeeze() throws IOException { - if (last != -1) { - outputStream.write(last); - } - last = -1; - } -} diff --git a/pgpainless-core/src/main/java/org/pgpainless/decryption_verification/OpenPgpMessageInputStream.java b/pgpainless-core/src/main/java/org/pgpainless/decryption_verification/OpenPgpMessageInputStream.java index 8d370965..75359de1 100644 --- a/pgpainless-core/src/main/java/org/pgpainless/decryption_verification/OpenPgpMessageInputStream.java +++ b/pgpainless-core/src/main/java/org/pgpainless/decryption_verification/OpenPgpMessageInputStream.java @@ -11,7 +11,6 @@ import java.io.OutputStream; import java.util.ArrayList; import java.util.Collection; import java.util.List; -import java.util.NoSuchElementException; import java.util.Stack; import org.bouncycastle.bcpg.BCPGInputStream; @@ -57,6 +56,8 @@ import org.pgpainless.util.Tuple; import org.slf4j.Logger; import org.slf4j.LoggerFactory; +import javax.annotation.Nonnull; + public class OpenPgpMessageInputStream extends InputStream { private static final Logger LOGGER = LoggerFactory.getLogger(OpenPgpMessageInputStream.class); @@ -66,16 +67,15 @@ public class OpenPgpMessageInputStream extends InputStream { protected final OpenPgpMetadata.Builder resultBuilder; // Pushdown Automaton to verify validity of OpenPGP packet sequence in an OpenPGP message protected final PDA automaton = new PDA(); - protected final DelayedTeeInputStreamInputStream delayedTee; - // InputStream of OpenPGP packets of the current layer - protected final BCPGInputStream packetInputStream; + // InputStream of OpenPGP packets + protected TeeBCPGInputStream packetInputStream; // InputStream of a nested data packet protected InputStream nestedInputStream; private boolean closed = false; private final Signatures signatures; - private MessageMetadata.Layer metadata; + private final MessageMetadata.Layer metadata; public OpenPgpMessageInputStream(InputStream inputStream, ConsumerOptions options) throws IOException, PGPException { @@ -95,9 +95,7 @@ public class OpenPgpMessageInputStream extends InputStream { this.signatures.addDetachedSignatures(options.getDetachedSignatures()); } - delayedTee = new DelayedTeeInputStreamInputStream(inputStream, signatures); - BCPGInputStream bcpg = BCPGInputStream.wrap(delayedTee); - this.packetInputStream = bcpg; + packetInputStream = new TeeBCPGInputStream(BCPGInputStream.wrap(inputStream), signatures); // *omnomnom* consumePackets(); @@ -122,41 +120,36 @@ public class OpenPgpMessageInputStream extends InputStream { private void consumePackets() throws IOException, PGPException { OpenPgpPacket nextPacket; - loop: while ((nextPacket = nextPacketTag()) != null) { + loop: while ((nextPacket = packetInputStream.nextPacketTag()) != null) { signatures.nextPacket(nextPacket); switch (nextPacket) { // Literal Data - the literal data content is the new input stream case LIT: automaton.next(InputAlphabet.LiteralData); - delayedTee.squeeze(); processLiteralData(); break loop; // Compressed Data - the content contains another OpenPGP message case COMP: automaton.next(InputAlphabet.CompressedData); - signatures.commitNested(); processCompressedData(); - delayedTee.squeeze(); break loop; // One Pass Signature case OPS: - automaton.next(InputAlphabet.OnePassSignatures); + automaton.next(InputAlphabet.OnePassSignature); PGPOnePassSignature onePassSignature = readOnePassSignature(); - delayedTee.squeeze(); signatures.addOnePassSignature(onePassSignature); break; // Signature - either prepended to the message, or corresponding to a One Pass Signature case SIG: + // true if Signature corresponds to OnePassSignature boolean isSigForOPS = automaton.peekStack() == StackAlphabet.ops; - automaton.next(InputAlphabet.Signatures); + automaton.next(InputAlphabet.Signature); - PGPSignature signature = readSignature(); - delayedTee.squeeze(); - processSignature(signature, isSigForOPS); + processSignature(isSigForOPS); break; @@ -166,7 +159,6 @@ public class OpenPgpMessageInputStream extends InputStream { case SED: case SEIPD: automaton.next(InputAlphabet.EncryptedData); - delayedTee.squeeze(); if (processEncryptedData()) { break loop; } @@ -175,8 +167,7 @@ public class OpenPgpMessageInputStream extends InputStream { // Marker Packets need to be skipped and ignored case MARKER: - packetInputStream.readPacket(); // skip - delayedTee.squeeze(); + packetInputStream.readMarker(); break; // Key Packets are illegal in this context @@ -204,26 +195,10 @@ public class OpenPgpMessageInputStream extends InputStream { } } - private OpenPgpPacket nextPacketTag() throws IOException { - int tag = nextTag(); - if (tag == -1) { - log("EOF"); - return null; - } - OpenPgpPacket packet; - try { - packet = OpenPgpPacket.requireFromTag(tag); - } catch (NoSuchElementException e) { - log("Invalid tag: " + tag); - throw e; - } - log(packet.toString()); - return packet; - } - - private void processSignature(PGPSignature signature, boolean isSigForOPS) { + private void processSignature(boolean isSigForOPS) throws PGPException, IOException { + PGPSignature signature = readSignature(); if (isSigForOPS) { - signatures.popNested(); + signatures.leaveNesting(); // TODO: Only leave nesting if all OPSs are dealt with signatures.addCorrespondingOnePassSignature(signature); } else { signatures.addPrependedSignature(signature); @@ -231,21 +206,22 @@ public class OpenPgpMessageInputStream extends InputStream { } private void processCompressedData() throws IOException, PGPException { - PGPCompressedData compressedData = new PGPCompressedData(packetInputStream); + signatures.enterNesting(); + PGPCompressedData compressedData = packetInputStream.readCompressedData(); MessageMetadata.CompressedData compressionLayer = new MessageMetadata.CompressedData( CompressionAlgorithm.fromId(compressedData.getAlgorithm())); nestedInputStream = new OpenPgpMessageInputStream(compressedData.getDataStream(), options, compressionLayer); } private void processLiteralData() throws IOException { - PGPLiteralData literalData = new PGPLiteralData(packetInputStream); + PGPLiteralData literalData = packetInputStream.readLiteralData(); this.metadata.setChild(new MessageMetadata.LiteralData(literalData.getFileName(), literalData.getModificationTime(), StreamEncoding.requireFromCode(literalData.getFormat()))); nestedInputStream = literalData.getDataStream(); } private boolean processEncryptedData() throws IOException, PGPException { - PGPEncryptedDataList encDataList = new PGPEncryptedDataList(packetInputStream); + PGPEncryptedDataList encDataList = packetInputStream.readEncryptedDataList(); // TODO: Replace with !encDataList.isIntegrityProtected() if (!encDataList.get(0).isIntegrityProtected()) { @@ -345,19 +321,6 @@ public class OpenPgpMessageInputStream extends InputStream { return false; } - private int nextTag() throws IOException { - try { - return packetInputStream.nextPacketTag(); - } catch (IOException e) { - if ("Stream closed".equals(e.getMessage())) { - // ZipInflater Streams sometimes close under our feet -.- - // Therefore we catch resulting IOEs and return -1 instead. - return -1; - } - throw e; - } - } - private List> findPotentialDecryptionKeys(PGPPublicKeyEncryptedData pkesk) { int algorithm = pkesk.getAlgorithm(); List> decryptionKeyCandidates = new ArrayList<>(); @@ -387,12 +350,12 @@ public class OpenPgpMessageInputStream extends InputStream { private PGPOnePassSignature readOnePassSignature() throws PGPException, IOException { - return new PGPOnePassSignature(packetInputStream); + return packetInputStream.readOnePassSignature(); } private PGPSignature readSignature() throws PGPException, IOException { - return new PGPSignature(packetInputStream); + return packetInputStream.readSignature(); } @Override @@ -428,7 +391,7 @@ public class OpenPgpMessageInputStream extends InputStream { } @Override - public int read(byte[] b, int off, int len) + public int read(@Nonnull byte[] b, int off, int len) throws IOException { if (nestedInputStream == null) { @@ -482,8 +445,7 @@ public class OpenPgpMessageInputStream extends InputStream { private void collectMetadata() { if (nestedInputStream instanceof OpenPgpMessageInputStream) { OpenPgpMessageInputStream child = (OpenPgpMessageInputStream) nestedInputStream; - MessageMetadata.Layer childLayer = child.metadata; - this.metadata.setChild((MessageMetadata.Nested) childLayer); + this.metadata.setChild((MessageMetadata.Nested) child.metadata); } } @@ -496,9 +458,9 @@ public class OpenPgpMessageInputStream extends InputStream { private static class SortedESKs { - private List skesks = new ArrayList<>(); - private List pkesks = new ArrayList<>(); - private List anonPkesks = new ArrayList<>(); + private final List skesks = new ArrayList<>(); + private final List pkesks = new ArrayList<>(); + private final List anonPkesks = new ArrayList<>(); SortedESKs(PGPEncryptedDataList esks) { for (PGPEncryptedData esk : esks) { @@ -526,11 +488,10 @@ public class OpenPgpMessageInputStream extends InputStream { } } - // TODO: In 'OPS LIT("Foo") SIG', OPS is only updated with "Foo" - // In 'OPS[1] OPS LIT("Foo") SIG SIG', OPS[1] (nested) is updated with OPS LIT("Foo") SIG. - // Therefore, we need to handle the innermost signature layer differently when updating with Literal data. - // For this we might want to provide two update entries into the Signatures class, one for OpenPGP packets and one - // for literal data. UUUUUGLY!!!! + // In 'OPS LIT("Foo") SIG', OPS is only updated with "Foo" + // In 'OPS[1] OPS LIT("Foo") SIG SIG', OPS[1] (nested) is updated with OPS LIT("Foo") SIG. + // Therefore, we need to handle the innermost signature layer differently when updating with Literal data. + // Furthermore, For 'OPS COMP(LIT("Foo")) SIG', the signature is updated with "Foo". CHAOS!!! private static final class Signatures extends OutputStream { final ConsumerOptions options; final List detachedSignatures; @@ -578,7 +539,7 @@ public class OpenPgpMessageInputStream extends InputStream { literalOPS.add(ops); if (signature.isContaining()) { - commitNested(); + enterNesting(); } } @@ -599,12 +560,12 @@ public class OpenPgpMessageInputStream extends InputStream { } } - void commitNested() { + void enterNesting() { opsUpdateStack.push(literalOPS); literalOPS = new ArrayList<>(); } - void popNested() { + void leaveNesting() { if (opsUpdateStack.isEmpty()) { return; } @@ -722,7 +683,7 @@ public class OpenPgpMessageInputStream extends InputStream { } @Override - public void write(byte[] b, int off, int len) { + public void write(@Nonnull byte[] b, int off, int len) { updatePacket(b, off, len); } diff --git a/pgpainless-core/src/main/java/org/pgpainless/decryption_verification/TeeBCPGInputStream.java b/pgpainless-core/src/main/java/org/pgpainless/decryption_verification/TeeBCPGInputStream.java new file mode 100644 index 00000000..f80793f0 --- /dev/null +++ b/pgpainless-core/src/main/java/org/pgpainless/decryption_verification/TeeBCPGInputStream.java @@ -0,0 +1,131 @@ +// SPDX-FileCopyrightText: 2022 Paul Schaub +// +// SPDX-License-Identifier: Apache-2.0 + +package org.pgpainless.decryption_verification; + +import org.bouncycastle.bcpg.BCPGInputStream; +import org.bouncycastle.bcpg.MarkerPacket; +import org.bouncycastle.bcpg.Packet; +import org.bouncycastle.openpgp.PGPCompressedData; +import org.bouncycastle.openpgp.PGPEncryptedDataList; +import org.bouncycastle.openpgp.PGPException; +import org.bouncycastle.openpgp.PGPLiteralData; +import org.bouncycastle.openpgp.PGPOnePassSignature; +import org.bouncycastle.openpgp.PGPSignature; +import org.pgpainless.algorithm.OpenPgpPacket; + +import java.io.IOException; +import java.io.InputStream; +import java.io.OutputStream; +import java.util.NoSuchElementException; + +/** + * Since we need to update signatures with data from the underlying stream, this class is used to tee out the data. + * Unfortunately we cannot simply override {@link BCPGInputStream#read()} to tee the data out though, since + * {@link BCPGInputStream#readPacket()} inconsistently calls a mix of {@link BCPGInputStream#read()} and + * {@link InputStream#read()} of the underlying stream. This would cause the second length byte to get swallowed up. + * + * Therefore, this class delegates the teeing to an {@link DelayedTeeInputStreamInputStream} which wraps the underlying + * stream. Since calling {@link BCPGInputStream#nextPacketTag()} reads up to and including the next packets tag, + * we need to delay teeing out that byte to signature verifiers. + * Hence, the reading methods of the {@link TeeBCPGInputStream} handle pushing this byte to the output stream using + * {@link DelayedTeeInputStreamInputStream#squeeze()}. + */ +public class TeeBCPGInputStream { + + protected final DelayedTeeInputStreamInputStream delayedTee; + // InputStream of OpenPGP packets of the current layer + protected final BCPGInputStream packetInputStream; + + public TeeBCPGInputStream(BCPGInputStream inputStream, OutputStream outputStream) { + this.delayedTee = new DelayedTeeInputStreamInputStream(inputStream, outputStream); + this.packetInputStream = BCPGInputStream.wrap(delayedTee); + } + + public OpenPgpPacket nextPacketTag() throws IOException { + int tag = packetInputStream.nextPacketTag(); + if (tag == -1) { + return null; + } + + OpenPgpPacket packet; + try { + packet = OpenPgpPacket.requireFromTag(tag); + } catch (NoSuchElementException e) { + throw e; + } + return packet; + } + + public Packet readPacket() throws IOException { + return packetInputStream.readPacket(); + } + + public PGPCompressedData readCompressedData() throws IOException { + delayedTee.squeeze(); + PGPCompressedData compressedData = new PGPCompressedData(packetInputStream); + return compressedData; + } + + public PGPLiteralData readLiteralData() throws IOException { + delayedTee.squeeze(); + return new PGPLiteralData(packetInputStream); + } + + public PGPEncryptedDataList readEncryptedDataList() throws IOException { + delayedTee.squeeze(); + return new PGPEncryptedDataList(packetInputStream); + } + + public PGPOnePassSignature readOnePassSignature() throws PGPException, IOException { + PGPOnePassSignature onePassSignature = new PGPOnePassSignature(packetInputStream); + delayedTee.squeeze(); + return onePassSignature; + } + + public PGPSignature readSignature() throws PGPException, IOException { + PGPSignature signature = new PGPSignature(packetInputStream); + delayedTee.squeeze(); + return signature; + } + + public MarkerPacket readMarker() throws IOException { + MarkerPacket markerPacket = (MarkerPacket) readPacket(); + delayedTee.squeeze(); + return markerPacket; + } + + public static class DelayedTeeInputStreamInputStream extends InputStream { + + private int last = -1; + private final InputStream inputStream; + private final OutputStream outputStream; + + public DelayedTeeInputStreamInputStream(InputStream inputStream, OutputStream outputStream) { + this.inputStream = inputStream; + this.outputStream = outputStream; + } + + @Override + public int read() throws IOException { + if (last != -1) { + outputStream.write(last); + } + last = inputStream.read(); + return last; + } + + /** + * Squeeze the last byte out and update the output stream. + * + * @throws IOException in case of an IO error + */ + public void squeeze() throws IOException { + if (last != -1) { + outputStream.write(last); + } + last = -1; + } + } +} diff --git a/pgpainless-core/src/main/java/org/pgpainless/decryption_verification/automaton/InputAlphabet.java b/pgpainless-core/src/main/java/org/pgpainless/decryption_verification/automaton/InputAlphabet.java index 8e795f5b..ad2a8c55 100644 --- a/pgpainless-core/src/main/java/org/pgpainless/decryption_verification/automaton/InputAlphabet.java +++ b/pgpainless-core/src/main/java/org/pgpainless/decryption_verification/automaton/InputAlphabet.java @@ -18,11 +18,11 @@ public enum InputAlphabet { /** * A {@link PGPSignatureList} object. */ - Signatures, + Signature, /** * A {@link PGPOnePassSignatureList} object. */ - OnePassSignatures, + OnePassSignature, /** * A {@link PGPCompressedData} packet. * The contents of this packet MUST form a valid OpenPGP message, so a nested PDA is opened to verify diff --git a/pgpainless-core/src/main/java/org/pgpainless/decryption_verification/automaton/PDA.java b/pgpainless-core/src/main/java/org/pgpainless/decryption_verification/automaton/PDA.java index 1d4b1189..156dc2ed 100644 --- a/pgpainless-core/src/main/java/org/pgpainless/decryption_verification/automaton/PDA.java +++ b/pgpainless-core/src/main/java/org/pgpainless/decryption_verification/automaton/PDA.java @@ -35,11 +35,11 @@ public class PDA { case LiteralData: return LiteralMessage; - case Signatures: + case Signature: automaton.pushStack(msg); return OpenPgpMessage; - case OnePassSignatures: + case OnePassSignature: automaton.pushStack(ops); automaton.pushStack(msg); return OpenPgpMessage; @@ -63,7 +63,7 @@ public class PDA { StackAlphabet stackItem = automaton.popStack(); switch (input) { - case Signatures: + case Signature: if (stackItem == ops) { return CorrespondingSignature; } else { @@ -78,7 +78,7 @@ public class PDA { } case LiteralData: - case OnePassSignatures: + case OnePassSignature: case CompressedData: case EncryptedData: default: @@ -92,7 +92,7 @@ public class PDA { State transition(InputAlphabet input, PDA automaton) throws MalformedOpenPgpMessageException { StackAlphabet stackItem = automaton.popStack(); switch (input) { - case Signatures: + case Signature: if (stackItem == ops) { return CorrespondingSignature; } else { @@ -107,7 +107,7 @@ public class PDA { } case LiteralData: - case OnePassSignatures: + case OnePassSignature: case CompressedData: case EncryptedData: default: @@ -121,7 +121,7 @@ public class PDA { State transition(InputAlphabet input, PDA automaton) throws MalformedOpenPgpMessageException { StackAlphabet stackItem = automaton.popStack(); switch (input) { - case Signatures: + case Signature: if (stackItem == ops) { return CorrespondingSignature; } else { @@ -136,7 +136,7 @@ public class PDA { } case LiteralData: - case OnePassSignatures: + case OnePassSignature: case CompressedData: case EncryptedData: default: @@ -156,7 +156,7 @@ public class PDA { // premature end of stream throw new MalformedOpenPgpMessageException(this, input, stackItem); } - } else if (input == InputAlphabet.Signatures) { + } else if (input == InputAlphabet.Signature) { if (stackItem == ops) { return CorrespondingSignature; } diff --git a/pgpainless-core/src/test/java/org/pgpainless/decryption_verification/OpenPgpMessageInputStreamTest.java b/pgpainless-core/src/test/java/org/pgpainless/decryption_verification/OpenPgpMessageInputStreamTest.java index 46c521ef..1955eebe 100644 --- a/pgpainless-core/src/test/java/org/pgpainless/decryption_verification/OpenPgpMessageInputStreamTest.java +++ b/pgpainless-core/src/test/java/org/pgpainless/decryption_verification/OpenPgpMessageInputStreamTest.java @@ -49,7 +49,6 @@ import org.pgpainless.util.Tuple; public class OpenPgpMessageInputStreamTest { - public static final String KEY = "-----BEGIN PGP PRIVATE KEY BLOCK-----\n" + "Version: PGPainless\n" + "Comment: DA05 848F 37D4 68E6 F982 C889 7A70 1FC6 904D 3F4C\n" + @@ -241,7 +240,7 @@ public class OpenPgpMessageInputStreamTest { System.out); } - public static void genSIG_LIT() throws PGPException, IOException { + public static void genSIG_COMP_LIT() throws PGPException, IOException { PGPSecretKeyRing secretKeys = PGPainless.readKeyRing().secretKeyRing(KEY); ByteArrayOutputStream msgOut = new ByteArrayOutputStream(); EncryptionStream signer = PGPainless.encryptAndOrSign() diff --git a/pgpainless-core/src/test/java/org/pgpainless/decryption_verification/automaton/PDATest.java b/pgpainless-core/src/test/java/org/pgpainless/decryption_verification/automaton/PDATest.java index 6e8a38d6..6dbb2cd6 100644 --- a/pgpainless-core/src/test/java/org/pgpainless/decryption_verification/automaton/PDATest.java +++ b/pgpainless-core/src/test/java/org/pgpainless/decryption_verification/automaton/PDATest.java @@ -30,9 +30,9 @@ public class PDATest { @Test public void testSimpleOpsSignedMesssageIsValid() throws MalformedOpenPgpMessageException { PDA automaton = new PDA(); - automaton.next(InputAlphabet.OnePassSignatures); + automaton.next(InputAlphabet.OnePassSignature); automaton.next(InputAlphabet.LiteralData); - automaton.next(InputAlphabet.Signatures); + automaton.next(InputAlphabet.Signature); automaton.next(InputAlphabet.EndOfSequence); assertTrue(automaton.isValid()); @@ -47,7 +47,7 @@ public class PDATest { @Test public void testSimplePrependSignedMessageIsValid() throws MalformedOpenPgpMessageException { PDA automaton = new PDA(); - automaton.next(InputAlphabet.Signatures); + automaton.next(InputAlphabet.Signature); automaton.next(InputAlphabet.LiteralData); automaton.next(InputAlphabet.EndOfSequence); @@ -63,10 +63,10 @@ public class PDATest { @Test public void testOPSSignedCompressedMessageIsValid() throws MalformedOpenPgpMessageException { PDA automaton = new PDA(); - automaton.next(InputAlphabet.OnePassSignatures); + automaton.next(InputAlphabet.OnePassSignature); automaton.next(InputAlphabet.CompressedData); // Here would be a nested PDA for the LiteralData packet - automaton.next(InputAlphabet.Signatures); + automaton.next(InputAlphabet.Signature); automaton.next(InputAlphabet.EndOfSequence); assertTrue(automaton.isValid());