From b5a304895fa39de0f714332e04fb42223874a2f0 Mon Sep 17 00:00:00 2001 From: Peter Dettman Date: Tue, 19 Mar 2024 15:36:02 +0700 Subject: Sanity checks and refactoring in Bcpg.Sig --- crypto/src/bcpg/sig/Exportable.cs | 23 ++------ crypto/src/bcpg/sig/KeyExpirationTime.cs | 21 +++---- crypto/src/bcpg/sig/PrimaryUserId.cs | 23 ++------ crypto/src/bcpg/sig/Revocable.cs | 23 ++------ crypto/src/bcpg/sig/SignatureExpirationTime.cs | 12 ++-- crypto/src/bcpg/sig/Utilities.cs | 41 ++++++++++++++ crypto/test/src/openpgp/test/BytesBooleansTest.cs | 68 +++++++++++++++++++++++ 7 files changed, 133 insertions(+), 78 deletions(-) create mode 100644 crypto/src/bcpg/sig/Utilities.cs create mode 100644 crypto/test/src/openpgp/test/BytesBooleansTest.cs diff --git a/crypto/src/bcpg/sig/Exportable.cs b/crypto/src/bcpg/sig/Exportable.cs index 8a9eafe09..ee4c98b24 100644 --- a/crypto/src/bcpg/sig/Exportable.cs +++ b/crypto/src/bcpg/sig/Exportable.cs @@ -1,5 +1,3 @@ -using System; - namespace Org.BouncyCastle.Bcpg.Sig { /** @@ -8,29 +6,16 @@ namespace Org.BouncyCastle.Bcpg.Sig public class Exportable : SignatureSubpacket { - private static byte[] BooleanToByteArray(bool val) - { - return new byte[1]{ Convert.ToByte(val) }; - } - - public Exportable( - bool critical, - bool isLongLength, - byte[] data) + public Exportable(bool critical, bool isLongLength, byte[] data) : base(SignatureSubpacketTag.Exportable, critical, isLongLength, data) { } - public Exportable( - bool critical, - bool isExportable) - : base(SignatureSubpacketTag.Exportable, critical, false, BooleanToByteArray(isExportable)) + public Exportable(bool critical, bool isExportable) + : base(SignatureSubpacketTag.Exportable, critical, false, Utilities.BooleanToBytes(isExportable)) { } - public bool IsExportable() - { - return data[0] != 0; - } + public bool IsExportable() => Utilities.BooleanFromBytes(data); } } diff --git a/crypto/src/bcpg/sig/KeyExpirationTime.cs b/crypto/src/bcpg/sig/KeyExpirationTime.cs index 62184b2a1..e8e5d916f 100644 --- a/crypto/src/bcpg/sig/KeyExpirationTime.cs +++ b/crypto/src/bcpg/sig/KeyExpirationTime.cs @@ -1,4 +1,4 @@ -using Org.BouncyCastle.Crypto.Utilities; +using System; namespace Org.BouncyCastle.Bcpg.Sig { @@ -8,23 +8,16 @@ namespace Org.BouncyCastle.Bcpg.Sig public class KeyExpirationTime : SignatureSubpacket { - protected static byte[] TimeToBytes(long t) - { - return Pack.UInt32_To_BE((uint)t); - } + [Obsolete("Will be removed")] + protected static byte[] TimeToBytes(long t) => Utilities.TimeToBytes((uint)t); - public KeyExpirationTime( - bool critical, - bool isLongLength, - byte[] data) + public KeyExpirationTime(bool critical, bool isLongLength, byte[] data) : base(SignatureSubpacketTag.KeyExpireTime, critical, isLongLength, data) { } - public KeyExpirationTime( - bool critical, - long seconds) - : base(SignatureSubpacketTag.KeyExpireTime, critical, false, TimeToBytes(seconds)) + public KeyExpirationTime(bool critical, long seconds) + : base(SignatureSubpacketTag.KeyExpireTime, critical, false, Utilities.TimeToBytes((uint)seconds)) { } @@ -33,6 +26,6 @@ namespace Org.BouncyCastle.Bcpg.Sig * * @return second count for key validity. */ - public long Time => (long)Pack.BE_To_UInt32(data); + public long Time => (long)Utilities.TimeFromBytes(data); } } diff --git a/crypto/src/bcpg/sig/PrimaryUserId.cs b/crypto/src/bcpg/sig/PrimaryUserId.cs index 184dfe8f7..f1637a2b9 100644 --- a/crypto/src/bcpg/sig/PrimaryUserId.cs +++ b/crypto/src/bcpg/sig/PrimaryUserId.cs @@ -1,5 +1,3 @@ -using System; - namespace Org.BouncyCastle.Bcpg.Sig { /** @@ -8,29 +6,16 @@ namespace Org.BouncyCastle.Bcpg.Sig public class PrimaryUserId : SignatureSubpacket { - private static byte[] BooleanToByteArray(bool val) - { - return new byte[1]{ Convert.ToByte(val) }; - } - - public PrimaryUserId( - bool critical, - bool isLongLength, - byte[] data) + public PrimaryUserId(bool critical, bool isLongLength, byte[] data) : base(SignatureSubpacketTag.PrimaryUserId, critical, isLongLength, data) { } - public PrimaryUserId( - bool critical, - bool isPrimaryUserId) - : base(SignatureSubpacketTag.PrimaryUserId, critical, false, BooleanToByteArray(isPrimaryUserId)) + public PrimaryUserId(bool critical, bool isPrimaryUserId) + : base(SignatureSubpacketTag.PrimaryUserId, critical, false, Utilities.BooleanToBytes(isPrimaryUserId)) { } - public bool IsPrimaryUserId() - { - return data[0] != 0; - } + public bool IsPrimaryUserId() => Utilities.BooleanFromBytes(data); } } diff --git a/crypto/src/bcpg/sig/Revocable.cs b/crypto/src/bcpg/sig/Revocable.cs index 6ded4d865..88dbf5195 100644 --- a/crypto/src/bcpg/sig/Revocable.cs +++ b/crypto/src/bcpg/sig/Revocable.cs @@ -1,5 +1,3 @@ -using System; - namespace Org.BouncyCastle.Bcpg.Sig { /** @@ -8,29 +6,16 @@ namespace Org.BouncyCastle.Bcpg.Sig public class Revocable : SignatureSubpacket { - private static byte[] BooleanToByteArray(bool value) - { - return new byte[1]{ Convert.ToByte(value) }; - } - - public Revocable( - bool critical, - bool isLongLength, - byte[] data) + public Revocable(bool critical, bool isLongLength, byte[] data) : base(SignatureSubpacketTag.Revocable, critical, isLongLength, data) { } - public Revocable( - bool critical, - bool isRevocable) - : base(SignatureSubpacketTag.Revocable, critical, false, BooleanToByteArray(isRevocable)) + public Revocable(bool critical, bool isRevocable) + : base(SignatureSubpacketTag.Revocable, critical, false, Utilities.BooleanToBytes(isRevocable)) { } - public bool IsRevocable() - { - return data[0] != 0; - } + public bool IsRevocable() => Utilities.BooleanFromBytes(data); } } diff --git a/crypto/src/bcpg/sig/SignatureExpirationTime.cs b/crypto/src/bcpg/sig/SignatureExpirationTime.cs index 44c714f33..eb2faede9 100644 --- a/crypto/src/bcpg/sig/SignatureExpirationTime.cs +++ b/crypto/src/bcpg/sig/SignatureExpirationTime.cs @@ -1,4 +1,4 @@ -using Org.BouncyCastle.Crypto.Utilities; +using System; namespace Org.BouncyCastle.Bcpg.Sig { @@ -8,10 +8,8 @@ namespace Org.BouncyCastle.Bcpg.Sig public class SignatureExpirationTime : SignatureSubpacket { - protected static byte[] TimeToBytes(long t) - { - return Pack.UInt32_To_BE((uint)t); - } + [Obsolete("Will be removed")] + protected static byte[] TimeToBytes(long t) => Utilities.TimeToBytes((uint)t); public SignatureExpirationTime(bool critical, bool isLongLength, byte[] data) : base(SignatureSubpacketTag.ExpireTime, critical, isLongLength, data) @@ -19,13 +17,13 @@ namespace Org.BouncyCastle.Bcpg.Sig } public SignatureExpirationTime(bool critical, long seconds) - : base(SignatureSubpacketTag.ExpireTime, critical, false, TimeToBytes(seconds)) + : base(SignatureSubpacketTag.ExpireTime, critical, false, Utilities.TimeToBytes((uint)seconds)) { } /** * return time in seconds before signature expires after creation time. */ - public long Time => Pack.BE_To_UInt32(data, 0); + public long Time => (long)Utilities.TimeFromBytes(data); } } diff --git a/crypto/src/bcpg/sig/Utilities.cs b/crypto/src/bcpg/sig/Utilities.cs new file mode 100644 index 000000000..2c0c314d9 --- /dev/null +++ b/crypto/src/bcpg/sig/Utilities.cs @@ -0,0 +1,41 @@ +using System; + +using Org.BouncyCastle.Crypto.Utilities; + +namespace Org.BouncyCastle.Bcpg.Sig +{ + internal static class Utilities + { + internal static bool BooleanFromBytes(byte[] bytes) + { + if (bytes.Length != 1) + throw new InvalidOperationException("Byte array has unexpected length. Expected length 1, got " + bytes.Length); + + if (bytes[0] == 0) + return false; + + if (bytes[0] == 1) + return true; + + throw new InvalidOperationException("Unexpected byte value for boolean encoding: " + bytes[0]); + } + + internal static byte[] BooleanToBytes(bool value) + { + return new byte[1]{ Convert.ToByte(value) }; + } + + internal static uint TimeFromBytes(byte[] bytes) + { + if (bytes.Length != 4) + throw new InvalidOperationException("Byte array has unexpected length. Expected length 4, got " + bytes.Length); + + return Pack.BE_To_UInt32(bytes); + } + + internal static byte[] TimeToBytes(uint t) + { + return Pack.UInt32_To_BE(t); + } + } +} diff --git a/crypto/test/src/openpgp/test/BytesBooleansTest.cs b/crypto/test/src/openpgp/test/BytesBooleansTest.cs new file mode 100644 index 000000000..3b6c8a577 --- /dev/null +++ b/crypto/test/src/openpgp/test/BytesBooleansTest.cs @@ -0,0 +1,68 @@ +using System; + +using NUnit.Framework; + +using Org.BouncyCastle.Bcpg.Sig; + +namespace Org.BouncyCastle.Bcpg.OpenPgp.Tests +{ + [TestFixture] + public class BytesBooleansTest + { + [Test] + public void TestParseFalse() + { + PrimaryUserId primaryUserID = new PrimaryUserId(true, false); + byte[] bFalse = primaryUserID.GetData(); + + Assert.AreEqual(1, bFalse.Length); + Assert.AreEqual(0, bFalse[0]); + Assert.False(primaryUserID.IsPrimaryUserId()); + } + + [Test] + public void TestParseTrue() + { + PrimaryUserId primaryUserID = new PrimaryUserId(true, true); + byte[] bTrue = primaryUserID.GetData(); + + Assert.AreEqual(1, bTrue.Length); + Assert.AreEqual(1, bTrue[0]); + Assert.True(primaryUserID.IsPrimaryUserId()); + } + + [Test] + public void TestParseTooShort() + { + PrimaryUserId primaryUserID = new PrimaryUserId(true, false, new byte[0]); + byte[] bTooShort = primaryUserID.GetData(); + + try + { + primaryUserID.IsPrimaryUserId(); + Assert.Fail("Should throw."); + } + catch (InvalidOperationException) + { + // expected. + } + } + + [Test] + public void TestParseTooLong() + { + PrimaryUserId primaryUserID = new PrimaryUserId(true, false, new byte[42]); + byte[] bTooLong = primaryUserID.GetData(); + + try + { + primaryUserID.IsPrimaryUserId(); + Assert.Fail("Should throw."); + } + catch (InvalidOperationException) + { + // expected. + } + } + } +} -- cgit 1.4.1