kmaragon / Konscious.Security.Cryptography

MIT License
202 stars 20 forks source link

Argon2i - write protected memory access with 32 Mb memory size #14

Closed ghost closed 7 years ago

ghost commented 7 years ago

I stumbled upon the very strange bug that is likely related to the hash algorithm implementation itself. The following code completely brakes NUnit test runner.

var argon2 = new Argon2i(passwordWithSalt);
argon2.DegreeOfParallelism = 4;
argon2.MemorySize = 32 * 1024;
argon2.Iterations = 4;
var hash = argon2.GetBytes(128);

It works like a charm while debugging but throws wild memory violation exceptions when I try to run it without debug mode.

Of course it may be rather NUnit or VS issue, BUT:

My only guess is that unmanaged algorithm implementation may be responsible for such strange side effects.

Environment: Windows 10, VS 2015 U3, .NET Framework 4.6, NUnit 3.6.0, NUnit3TestAdapter 3.7.0, Konscious.Security.Cryptography.Argon2 1.1.0

kmaragon commented 7 years ago

I'll try to look into this. I don't have a Windows 10 (or any other version) machine but maybe valgrind can detect any memory issues. For what it's worth, this is a 100% C# implementation. So it's definitely not using the same libraries as libsodium (as is obvious by the fact that this library supports all OSes that run .NET core, unlike libsodium). But it does do some unsafe memory stuff to translate bytes to qwords and to take advantage of JIT compilation hopefully generating some hardware vectorization instructions which the C library uses explicitly, knowing that it still wouldn't match the perf profile. But the implementation is definitely guided very precisely by the paper and the C reference implementation. So I very well may have reimplemented some kind of off by one corruption or something.

kmaragon commented 7 years ago

Doh - apparently I'm running too new a Linux Kernel to check. The dotnet library uses an rcu library that uses memory barriers which as a syscall was added in kernel 4.3.

https://bugs.kde.org/show_bug.cgi?id=361726#c7

I'm not sure if this is the specific issue that causes it to just deadlock at 100% CPU on a single core but it doesn't even start running any .NET code. :-(. I definitely have that issue though

ghost commented 7 years ago

I beg you pardon, Keef. It's all about the AOP framework I am using :-(. It doesn't support async methods weaving.

I looked through the sources of Aragon2.cs and found that synchronous GetBytes() method actually calls GetBytesAsync(bc).Result. AOP framework mixes the stack and CLR goes mad.

Sorry again. And thank you very much for this high quality Aragon2 implementation.

kmaragon commented 7 years ago

Oh interesting.

That's unfortunate. It ends up being one more piece of ammo in Anders' AOP Hate gun.

I quite like the concept of AOP in a managed language. Anders' says he doesn't like how it can change your code without you knowing and that's bad... but that's precisely what a managed environment does anyway. And AOP can make code really great and easy to follow / maintain.

At the risk of some disparagement, I'm curious which framework it was.

ghost commented 7 years ago

I'm using https://github.com/Virtuoze/NConcern. It is pretty new and unstable though, but one of the most elegant solutions I have seen. Dynamic code reweaving is still remains the challenging task. I have some experience on reweaving the properties using old good Mono.Cecil. The process itself has a lot of pitfalls, for example, Roslyn compiler generates code incompatible with earlier compiler versions.

Of course, Anders is right in general. However, sometimes AOP is the best solution. Instead of writing thousands lines of absolutely identical code you can just inject a tine aspect and forget about it. Automatic INotifyProperty implementation and implicit null argument check are among the most popular aspects..

In my case it is a call context setup.

I declare aspect:

public class ImplicitContextAspect : IAspect
{
    public IEnumerable<IAdvice> Advise(MethodInfo method)
    {
        foreach (var prop in method.DeclaringType.GetProperties(BindingFlags.Instance | BindingFlags.Public | BindingFlags.NonPublic))
        {
            if (prop.GetMethod == method || prop.SetMethod == method) yield break;
        }
        yield return Advice.Basic.Before((instance, arguments) =>
        {
            //set implicit context here
        });
    }
}

and then simply apply it to all classes that implement some interface (IEntityWrapper in my case):

Aspect.Weave<ImplicitContextAspect>(method => !method.IsStatic && typeof(IEntityWrapper).IsAssignableFrom(method.DeclaringType));

Now I can be sure that context is properly initialised before each call of any class that implements IEntityWrapper.

Therefore I'll try to implement async/await support for NConcern.

kmaragon commented 7 years ago

I see. I haven't used NConcern before I remember when PostSharp broke down with async/await. Although, they've managed to get it supported now. I'll check out NConcern though.

Thanks so much for the info!