Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Appropriately free temporary values on EC import #73078

Merged
merged 1 commit into from
Jul 29, 2022
Merged

Conversation

bartonjs
Copy link
Member

EC import from parameters calls into a series of set-routines that neither
self-describe their ownership of the reference counted inputs (set0 vs set1)
nor document them.

Looking at code usage examples, and underlying code, they are all logically
set1-style routines, in that they do not take ownership of the inputs, which
means we need to free the BN values on both success and failure. This assertion
was verified by an intermediate change that reset all numbers to 1
(e.g. BN_one(qxBn)) and watched for test failures. When none of our tests
failed with that change, it moved to the freeing form.

Verified with importing the cross-product of:
{ secp256r1, sect163k1 }
x
{ named curve, explicit parameters }
x
{ q + d, q only, d only }

Fixes #57528.

@ghost
Copy link

ghost commented Jul 29, 2022

Tagging subscribers to this area: @dotnet/area-system-security, @vcsjones
See info in area-owners.md if you want to be subscribed.

Issue Details

EC import from parameters calls into a series of set-routines that neither
self-describe their ownership of the reference counted inputs (set0 vs set1)
nor document them.

Looking at code usage examples, and underlying code, they are all logically
set1-style routines, in that they do not take ownership of the inputs, which
means we need to free the BN values on both success and failure. This assertion
was verified by an intermediate change that reset all numbers to 1
(e.g. BN_one(qxBn)) and watched for test failures. When none of our tests
failed with that change, it moved to the freeing form.

Verified with importing the cross-product of:
{ secp256r1, sect163k1 }
x
{ named curve, explicit parameters }
x
{ q + d, q only, d only }

Fixes #57528.

Author: bartonjs
Assignees: -
Labels:

area-System.Security

Milestone: -

@bartonjs
Copy link
Member Author

The glorious verifier program:

using System;
using System.Globalization;
using System.Security.Cryptography;

namespace Scratch
{
    internal static class Program
    {
        internal static void Main()
        {
#if SECP256R1
            ECParameters parameters = new ECParameters
            {
                Curve = ECCurve.NamedCurves.nistP256,
/*
                Curve = new ECCurve
                {
                    CurveType = ECCurve.ECCurveType.PrimeShortWeierstrass,
                    Prime = "FFFFFFFF00000001000000000000000000000000FFFFFFFFFFFFFFFFFFFFFFFF".HexToByteArray(),
                    A = "FFFFFFFF00000001000000000000000000000000FFFFFFFFFFFFFFFFFFFFFFFC".HexToByteArray(),
                    B = "5AC635D8AA3A93E7B3EBBD55769886BC651D06B0CC53B0F63BCE3C3E27D2604B".HexToByteArray(),
                    Seed = "C49D360886E704936A6678E1139D26B7819F7E90".HexToByteArray(),
                    Hash = HashAlgorithmName.SHA1,
                    G = new ECPoint
                    {
                        X = "6B17D1F2E12C4247F8BCE6E563A440F277037D812DEB33A0F4A13945D898C296".HexToByteArray(),
                        Y = "4FE342E2FE1A7F9B8EE7EB4A7C0F9E162BCE33576B315ECECBB6406837BF51F5".HexToByteArray(),
                    },
                    Order = "FFFFFFFF00000000FFFFFFFFFFFFFFFFBCE6FAADA7179E84F3B9CAC2FC632551".HexToByteArray(),
                    Cofactor = new byte[] { 0x01 },
                },
*/
                //Q = new ECPoint
                //{
                    //X = "8101ECE47464A6EAD70CF69A6E2BD3D88691A3262D22CBA4F7635EAFF26680A8".HexToByteArray()
                    //Y = "D8A12BA61D599235F67D9CB4D58F1783D3CA43E78F0A5ABAA624079936C0C3A9".HexToByteArray(),
                //},
                D = "70A12C2DB16845ED56FF68CFC21A472B3F04D7D6851BF6349F2D7D5B3452B38A".HexToByteArray(),
            };

            ReadOnlySpan<byte> signature = new byte[]
            {
                0x39, 0xA3, 0x13, 0x60, 0x73, 0x6B, 0x22, 0x39,
                0x6E, 0xDB, 0x12, 0x63, 0x2A, 0xEE, 0xD1, 0x16,
                0x70, 0xA3, 0x69, 0x6E, 0x3E, 0x41, 0x11, 0x43,
                0xBF, 0x40, 0x30, 0x98, 0x03, 0x35, 0x44, 0xD2,
                0xEB, 0x4C, 0x80, 0x2C, 0xC0, 0x37, 0x2B, 0x7E,
                0xA1, 0x83, 0xE3, 0x58, 0x80, 0x4B, 0x64, 0xB9,
                0x35, 0xEB, 0x29, 0xA5, 0x8D, 0x1B, 0xB2, 0x3E,
                0xEF, 0xE0, 0xCC, 0xB1, 0xD3, 0x86, 0x8E, 0x6E,
            };

#else
            ECParameters parameters = new ECParameters
            {
                Curve = ECCurve.CreateFromValue("1.3.132.0.1"),
                //Curve =
                //{
                    //CurveType = ECCurve.ECCurveType.Characteristic2,
                    //Polynomial = "0800000000000000000000000000000000000000C9".HexToByteArray(),
                    //A = "000000000000000000000000000000000000000001".HexToByteArray(),
                    //B = "000000000000000000000000000000000000000001".HexToByteArray(),
                    //G =
                    //{
                        //X = "02FE13C0537BBC11ACAA07D793DE4E6D5E5C94EEE8".HexToByteArray(),
                        //Y = "0289070FB05D38FF58321F2E800536D538CCDAA3D9".HexToByteArray(),
                    //},
                    //Order = "04000000000000000000020108A2E0CC0D99F8A5EF".HexToByteArray(),
                    //Cofactor = new byte[] { 2 },
                //},
                //Q =
                //{
                    //X = "06179E3719CC8125435FE64589D5FCA6DC1887551D".HexToByteArray(),
                    //Y = "0788FEBB62FFC9248D8B398AFE3E0B5C3C02B3FE37".HexToByteArray(),
                //},
                D = "03C1995ADFAE8C0518DC13DFE6304BB01017E6A412".HexToByteArray(),
            };

            ReadOnlySpan<byte> signature = new byte[]
            {
                0x00, 0x43, 0x19, 0xA9, 0xA7, 0x13, 0x0E, 0x25,
                0xB1, 0xB4, 0x37, 0xC5, 0x6D, 0xBA, 0xA5, 0x83,
                0x37, 0x86, 0x41, 0x18, 0x77, 0x02, 0x24, 0xC3,
                0xD5, 0x42, 0x96, 0xB3, 0x2A, 0xAB, 0x55, 0xF8,
                0xDC, 0x69, 0xF9, 0xEE, 0xFB, 0xCD, 0x37, 0x78,
                0x57, 0xDE,
            };

#endif

            Run(parameters, signature);
        }

        private static void Run(ECParameters parameters, ReadOnlySpan<byte> signature)
        {

            long lastMem = System.Diagnostics.Process.GetCurrentProcess().VirtualMemorySize64;

            const string Yes = "present";
            const string No = "absent";
            Console.WriteLine($"{parameters.Curve.CurveType}: Q:{(parameters.Q.X is null ? No : Yes)} D:{(parameters.D is null ? No : Yes)}");

            GC.Collect();
            GC.Collect();
            GC.WaitForPendingFinalizers();

            ReadOnlySpan<byte> hash = new byte[32];

            for (int iter = 0; iter < 50_000; iter++)
            {
                if ((iter & 1023) == 0)
                {
                    ShowMem(ref lastMem);
                }

                using (ECDsa key = ECDsa.Create(parameters))
                {
                    if (!key.VerifyHash(hash, signature))
                    {
                        if (parameters.D is not null)
                        {
                            byte[] realSig = key.SignHash(hash.ToArray());

                            Console.WriteLine("            ReadOnlySpan<byte> signature = new byte[]");
                            Console.WriteLine("            {");

                            for (int i = 0; i < realSig.Length; i += 8)
                            {
                                Console.Write("                ");

                                for (int j = i; j < i + 8 && j < realSig.Length; j++)
                                {
                                    Console.Write($"0x{realSig[j]:X2}, ");
                                }

                                Console.WriteLine();
                            }

                            Console.WriteLine("            };");
                        }

                        throw new InvalidOperationException();
                    }
                }
            }

            Console.WriteLine("Done.");
            ShowMem(ref lastMem);

            GC.Collect();
            GC.Collect();
            GC.WaitForPendingFinalizers();
            GC.Collect();
            GC.Collect();
            GC.WaitForPendingFinalizers();
            GC.Collect();

            Console.WriteLine("GC's done, too.");
            ShowMem(ref lastMem);

            static void ShowMem(ref long lastMem)
            {
                long thisMem = System.Diagnostics.Process.GetCurrentProcess().VirtualMemorySize64;
                long delta = thisMem - lastMem;
                lastMem = thisMem;

                if (delta > 0)
                {
                    Console.WriteLine($"+{delta}");
                }
                else
                {
                    Console.WriteLine(delta);
                }
            }
        }

        private static byte[] HexToByteArray(this string hexString)
        {
            return Convert.FromHexString(hexString);
        }
    }
}

@bartonjs bartonjs merged commit 1dc6524 into dotnet:main Jul 29, 2022
@bartonjs bartonjs deleted the ecmem branch July 29, 2022 21:55
@GrabYourPitchforks
Copy link
Member

LGTM also. Thanks! 🎉

@ghost ghost locked as resolved and limited conversation to collaborators Aug 29, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Possible memory leak in ECDiffieHellmanOpenSsl
3 participants