Add preliminary EdDSA validation support #239

ntadej · 2022-01-16T13:10:33Z

Add preliminary EdDSA validation support (partially closes #187 as signing is still missing). I tested with valid and invalid signature and it seems to work. Note that more testing is probably needed.

Some comments:

Uses https://github.com/orlp/ed25519.git like the primary Sparkle project, causes minimal size increase of the final dll.
Public key verfication is not there yet, I guess it is enough to just do the Base64 decoding + size check?
Not sure I updated bakefile/vcxproj correctly. I get some errors, but it everything looks fine. The only thing is that the format changed a bit (I guess I'm using a different version of bakefile as the current files were generated with).
Do we need that complicated file reading? I admit I took the first snippet from StackOverflow on how to read full files on windows.
RC integration not tested (I'm not primarily a Windows developer, I'm just trying to port one of my apps to Windows).

Tagging @kornelski and @vslavik.

Youw · 2022-01-16T16:10:03Z

src/signatureverifier.cpp

+std::vector<unsigned char> Base64ToUnsignedBin(const std::string& base64)
+{
+    std::string bin = Base64ToBin(base64);
+    return std::vector<unsigned char>(bin.data(), bin.data() + bin.length() + 1);
+}
+


see my comment below

Suggested change

std::vector<unsigned char> Base64ToUnsignedBin(const std::string& base64)

{

std::string bin = Base64ToBin(base64);

return std::vector<unsigned char>(bin.data(), bin.data() + bin.length() + 1);

}

Youw · 2022-01-16T16:20:46Z

src/signatureverifier.cpp

+    std::vector<unsigned char> signature = Base64ToUnsignedBin(signature_base64);
+    std::vector<unsigned char> key = Base64ToUnsignedBin(Settings::GetEdDSAPubKey());
+
+    int result = ed25519_verify(signature.data(),
+                                buffer,
+                                bytes_read,
+                                key.data());


no need in additional intermediate copy of the data here:

Suggested change

std::vector<unsigned char> signature = Base64ToUnsignedBin(signature_base64);

std::vector<unsigned char> key = Base64ToUnsignedBin(Settings::GetEdDSAPubKey());

int result = ed25519_verify(signature.data(),

buffer,

bytes_read,

key.data());

const std::string signature = Base64ToBin(signature_base64);

const std::string key = Base64ToBin(Settings::GetEdDSAPubKey());

int result = ed25519_verify(reinterpret_cast<const unsigned char*>(signature.data()),

buffer,

bytes_read,

reinterpret_cast<const unsigned char*>(key.data()));

Youw · 2022-01-16T16:49:59Z

src/signatureverifier.cpp

+
+    DWORD size = GetFileSize(payload, NULL);
+    DWORD bytes_read = 0;
+    unsigned char *buffer = new unsigned char[size + 1];


replace with std::vector<unsigned char>

Youw · 2022-01-16T16:51:04Z

src/signatureverifier.cpp

+    HANDLE payload;
+    payload = CreateFile(filename.c_str(), GENERIC_READ, FILE_SHARE_READ, NULL, OPEN_EXISTING, FILE_ATTRIBUTE_NORMAL, NULL);


Suggested change

HANDLE payload;

payload = CreateFile(filename.c_str(), GENERIC_READ, FILE_SHARE_READ, NULL, OPEN_EXISTING, FILE_ATTRIBUTE_NORMAL, NULL);

CFile payload (_wfopen(filename.c_str(), L"rb"));

always prefer using standard C/C++ (in this particular case _wfopen is not exactly standard C, but that is lesser evil in this case) instead of Platform-specific functionality, if possible
(sample: https://stackoverflow.com/a/14002993/3697939)

Youw · 2022-01-16T16:53:33Z

src/signatureverifier.cpp

+    // TODO: validate
+    (void)key;


This is how ed25519_verify reads they key: https://github.com/orlp/ed25519/blob/7fa6712ef5d581a6981ec2b08ee623314cd1d1c4/src/verify.c#L58
We can do exactly the same to verify the key.
Make sure to verify that the key len (after decode from base64) is exactly 32 bytes byfore passing into ge_frombytes_negate_vartime

Youw · 2022-01-16T16:54:20Z

src/signatureverifier.cpp

+    }
+    buffer[bytes_read] = '\0';
+
+    std::vector<unsigned char> signature = Base64ToUnsignedBin(signature_base64);


verify that the signature is exactly 64 bytes before passing it into ed25519_verify

Youw · 2022-01-16T16:55:06Z

src/signatureverifier.cpp

+    buffer[bytes_read] = '\0';
+
+    std::vector<unsigned char> signature = Base64ToUnsignedBin(signature_base64);
+    std::vector<unsigned char> key = Base64ToUnsignedBin(Settings::GetEdDSAPubKey());


verify that the key is exactly 32 bytes before passing it into ed25519_verify

purejava · 2023-01-07T17:40:58Z

@ntadej, @Youw, do you see any chance to proceed with this PR?
Thanks.

purejava · 2023-01-08T07:46:02Z

The reason why I am asking is, that I have created Java bindings for WinSparkle and we want to use these to update Cryptomator on Windows, that securely stores vaults in cloud services.

ntadej · 2023-01-08T10:59:18Z

I am planning to, but Windows is currently at the bottom of priority list for my project.

geraldcombs · 2023-05-05T20:44:11Z

I think I've fixed the issues identified by @Youw in https://github.com/geraldcombs/winsparkle/tree/eddsa-updates. The only outstanding issue is that SignatureVerifier::VerifyEdDSAPubKey is still a placeholder. I think the proper fix would be to add a corresponding function to the ed25519 public API.

@ntadej if my changes look OK to you, feel free to grab them and add them to this PR (and assign yourself authorship if you'd like). Otherwise I'd be happy to open a separate PR.

vslavik · 2023-05-06T14:23:48Z

@geraldcombs Thanks!

The only outstanding issue is that SignatureVerifier::VerifyEdDSAPubKey is still a placeholder. I think the proper fix would be to add a corresponding function to the ed25519 public API.

Let's make this simpler: VerifyEdDSAPubKey has little usefulness and the verification is done at signature checking time anyway, so we can either just remove this part or do what was initially suggested:

Public key verfication is not there yet, I guess it is enough to just do the Base64 decoding + size check?

vslavik · 2023-05-06T14:25:28Z

Otherwise I'd be happy to open a separate PR.

Oh, sorry, forgot to say: please do.

ntadej · 2023-05-06T14:27:37Z

Yes, sorry for lack of activity here. I also propose you open a separate PR so it will be easier to iterate for you.

geraldcombs · 2023-05-08T20:39:22Z

Oh, sorry, forgot to say: please do.

Done in #254.

vslavik · 2023-05-21T09:22:58Z

→ #254

Add preliminary EdDSA validation support

72c4114

Youw reviewed Jan 16, 2022

View reviewed changes

nrk-akjaer mentioned this pull request Jan 6, 2023

Feature: Autoupdate on all platforms, here: WinSparkle cryptomator/cryptomator#2596

Closed

geraldcombs mentioned this pull request May 8, 2023

EdDSA validation support #254

Open

vslavik closed this May 21, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add preliminary EdDSA validation support #239

Add preliminary EdDSA validation support #239

ntadej commented Jan 16, 2022

Youw Jan 16, 2022

Youw Jan 16, 2022

Youw Jan 16, 2022

Youw Jan 16, 2022 •

edited

Loading

Youw Jan 16, 2022

Youw Jan 16, 2022

Youw Jan 16, 2022

purejava commented Jan 7, 2023

purejava commented Jan 8, 2023

ntadej commented Jan 8, 2023

geraldcombs commented May 5, 2023

vslavik commented May 6, 2023

vslavik commented May 6, 2023

ntadej commented May 6, 2023

geraldcombs commented May 8, 2023

vslavik commented May 21, 2023

		HANDLE payload;
		payload = CreateFile(filename.c_str(), GENERIC_READ, FILE_SHARE_READ, NULL, OPEN_EXISTING, FILE_ATTRIBUTE_NORMAL, NULL);

	HANDLE payload;
	payload = CreateFile(filename.c_str(), GENERIC_READ, FILE_SHARE_READ, NULL, OPEN_EXISTING, FILE_ATTRIBUTE_NORMAL, NULL);
	CFile payload (_wfopen(filename.c_str(), L"rb"));

Add preliminary EdDSA validation support #239

Add preliminary EdDSA validation support #239

Conversation

ntadej commented Jan 16, 2022

Youw Jan 16, 2022

Choose a reason for hiding this comment

Youw Jan 16, 2022

Choose a reason for hiding this comment

Youw Jan 16, 2022

Choose a reason for hiding this comment

Youw Jan 16, 2022 • edited Loading

Choose a reason for hiding this comment

Youw Jan 16, 2022

Choose a reason for hiding this comment

Youw Jan 16, 2022

Choose a reason for hiding this comment

Youw Jan 16, 2022

Choose a reason for hiding this comment

purejava commented Jan 7, 2023

purejava commented Jan 8, 2023

ntadej commented Jan 8, 2023

geraldcombs commented May 5, 2023

vslavik commented May 6, 2023

vslavik commented May 6, 2023

ntadej commented May 6, 2023

geraldcombs commented May 8, 2023

vslavik commented May 21, 2023

Youw Jan 16, 2022 •

edited

Loading