charlesportwoodii / libsodium-uwp

libsodium for Universal Windows Platform (UWP) - A secure cryptographic library
BSD 3-Clause "New" or "Revised" License
16 stars 1 forks source link

[KDF:Argon2i/Scrypt] on x64 |System.AccessViolationException #21

Closed globeport closed 7 years ago

globeport commented 7 years ago

I'm getting a System.AccessViolationException raised executing the following in a fresh UWP app, referencing the latest libsodium-uwp nuget (1.0.12) running on x64 platform. The operation executes successfully running on x86. Any ideas? Thanks

var result = Sodium.KDF.Argon2i("test", Sodium.Core.GetRandomBytes(16), new Sodium.PasswordHashOptions { time_cost = 3, memory_cost = 16384 });

charlesportwoodii commented 7 years ago

@globeport Syntax error aside, I'm not able to reproduce this in a new UnitTestProject I just created using Nuget 1.0.12.

A couple of questions:

  1. Do you have a more verbose error you can provide?
  2. What is your build target? x64 Local Machine?
  3. Does your target have 16MiB of available RAM for Argon2i to fill?
  4. Does the problem persist if you include libsodium-uwp from a recursive clone rather than Nuget? (git clone --recursive https://github.com/charlesportwoodii/libsodium-uwp?
  5. Are you trying to generate a 32 byte key? Or do you just want to create an Argon2i hash for storage? PasswordHash would be better for the later.
globeport commented 7 years ago

Appologies for syntax error above.

  1. Slightly more information given in the exception:

'Attempted to read or write protected memory. This is often an indication that other memory is corrupt.'

  1. Yes, x64 LocalMachine

  2. 8gb ram desktop machine

  3. The error has only started to occur since switching to the nuget package. Prior to that I was referencing a clone and things worked fine.

  4. Yes I'm generating a 32 byte key for authentication

charlesportwoodii commented 7 years ago

@globeport FYI PasswordHashOptions.memory_cost is represented in KiB.16384 KiB => 16 MiB

Clone from git then instead. I'm not able to reproduce it but I'd bet that solves the problem for you. Nuget support for Universal Windows Runtime UWP packages still isn't that great. I'll revisit it again before 1.0.13.

globeport commented 7 years ago

Thanks. Tried reducing the memory cost but the error persists.

Sure I can go back to a clone. I was just attempting to clean up my solution a bit :)

Thanks.

charlesportwoodii commented 7 years ago

The .nuspec in master is newer than what was used for 1.0.12. If you want, you can try to see if packaging master gets you a workable nuspec file.

That process would look like (download nuget.exe if you don't have WSL bash installed)

cd /path/to/libsodium-uwp
bash -lc "wget https://dist.nuget.org/win-x86-commandline/v4.0.0/nuget.exe"
nuget.exe pack .nuget\libsodium-uwp.nuspec

Then in Visual Studio View->Other Windows->Package Manager Console

Install-Package C:\path\to\libsodium-uwp.1.0.12.nupkg

Still not sure why you're seeing the error and I'm not, but see if that works for you.

globeport commented 7 years ago

OK I'll have a look at this at somepoint today. Thanks

globeport commented 7 years ago

Same problem packaging with latest version of nuget. I can only get things to work if I link directly to the project. Strange. I'll let you know if I find a solution.

charlesportwoodii commented 7 years ago

@globeport That's mindboggling. If I upload a project a little later today would you be able to test it on your setup and let me know the output? It would just be a simple UnitTest project that loads the Nuget package and runs yours Argon2i call.

Out of curiosity, are you on VS2017 or VS2015?

globeport commented 7 years ago

Sure, I'm on VS2017

charlesportwoodii commented 7 years ago

@globeport

Then that's why. The nuget package is built against the Visual C++ Redistributable for Visual Studio 2015 which I don't think works right in VS2017. The package should compile fine, but the nuget package needs to be retargeted for the 2017 redist.

I'm not able to run VS2017 yet on my development machine, but as soon as I can port the project to VS2017 the nuget package should be fixed.

globeport commented 7 years ago

Great, thankyou.

globeport commented 7 years ago

Hi @charlesportwoodii, are you in a position to be able to publish a new release? I'm going to publish a nuget package with this library as a dependency and it would make life much easier if you could? Thanks.

charlesportwoodii commented 7 years ago

@globeport For VS2017 or VS2015?

globeport commented 7 years ago

2017

charlesportwoodii commented 7 years ago

Let me see what I can do. I had to uninstall VS2017 a week or 2 ago because of an issue and haven't re-installed it yet. I'll try to get back to you shortly.

charlesportwoodii commented 7 years ago

@globeport:

I was able to reproduce the issue for both Argon2i and Scrypt when using the nuget package for x64 builds. I tried making a new package for 2017 but I'm still seeing the same issue.

To be honest I'm not sure what the problem is though. I can't reproduce it in the test suite, and when I copy over the DLL's from the test suite to a sample project, I still see the same issue.

Give that the tests pass I'm pretty confident this has something to do with how Nuget packages the libraries rather than a code issue, however I really don't have the time presently to dig into what Nuget is doing. To be perfectly honest I'm not entirely convinced Nuget works right 100% of the time for Universal C Runtime Components.

If you're able to identify and fix the issue with the files in the .nuget directory I'd gladly welcome a Pull Request. However at the moment I don't have the time to keep digging into this.

I'll re-open the issue, but a MR would be the fastest way to get this published back to nuget.

globeport commented 7 years ago

OK I'll investigate this at somepoint. Thanks.

globeport commented 7 years ago

The Sodium.winmd reference in the nuspec file seems to be causing the problem. If I refer to the x64 build of Sodium.winmd instead, then everything works fine for x64 builds. So it does seem that the winmd file is platform specific. The question is how do you reference the correct version in nuspec?

charlesportwoodii commented 7 years ago

That's odd. According to the nuget docs the winmd is suppose to be platform independent and only contain the reference, not the implementation.

Should be able to reference that by changing the nuspec to pull in the platform specific one. I can try to rebuild and test that in an hour or so.

On Jun 8, 2017 07:15, "globeport" notifications@github.com wrote:

The Sodium.winmd reference in the nuspec file seems to be causing the problem. If I refer to the x64 build of Sodium.winmd instead, then everything works fine for x64 builds. So it does seem that the winmd file is platform specific. The question is how do you reference the correct version in nuspec?

— You are receiving this because you modified the open/close state. Reply to this email directly, view it on GitHub https://github.com/charlesportwoodii/libsodium-uwp/issues/21#issuecomment-307085604, or mute the thread https://github.com/notifications/unsubscribe-auth/AAmguWyXWy78b1Z2BQwGfoy9thSDu7jYks5sB-XYgaJpZM4M3_bI .

globeport commented 7 years ago

I have been trying but no luck. I should say I have finally managed to get my UWP nuget package working without having to reference the libsodium package so its no longer a priority for me but I guess it would be nice to figure this out.

charlesportwoodii commented 7 years ago

I see.

I think the problem actually had to do with the PasswordHashOptions struct not working correctly on x64 to be honest. I made a wrapper for it and suddenly x64 started working. Here's the relevant code block.

var str = "test";
var options = PasswordHash.CreateOptions(16384, 3);
var result = KDF.Argon2i(str, options);

I just push 1.0.122 to nuget (https://www.nuget.org/packages/libsodium-uwp/1.0.122). The package uploaded seems to work right on x86, x64, and the ARM device I have. It's still indexing on nuget, but once it's done - any chance you could confirm?

globeport commented 7 years ago

Yes that's working now. Hooray!

charlesportwoodii commented 7 years ago

Thanks for confirming! I've updated the relevant documentation for this as well.

If you find any other oddities let me know.

Thanks!