add LogCritical in empty catch block

[ghost]

No description provided.

[ghost]

@kiminuo can this be merged? Normal PR that should not have have any controversial opinions

[lontivero]

Password Finder is a tool for helping those users that could have forgotten one character of their password and then it tries to decrypt the wallet with all possible replacements of every single position on the password. Every time the password is incorrect a SecurityException and then we ignore it a try with the next one. For that reason it is expected to receive many millions of exceptions all saying Invalid password (or invalid Network).

[kiminuo]

@prayank23 Please have a look here: https://github.com/MetacoSA/NBitcoin/blob/a9b8e592fa7df0e97bdffe7de94ed58a87d92144/NBitcoin/BIP38/BitcoinEncryptedSecret.cs#L211. That NBitcoin's GetKey method is designed in a way that SecurityException is thrown when the provided password is incorrect. For WW use case, it would be better to have bool GetKey(string password, out Key? key) method (not implemented at the moment) and avoid throwing exceptions as that would be slightly faster for us and it would convey the meaning better.

SecurityException is may be a confusing name, I guess.

[ghost]

Password Finder is a tool for helping those users that could have forgotten one character of their password and then it tries to decrypt the wallet with all possible replacements of every single position on the password. Every time the password is incorrect a SecurityException and then we ignore it a try with the next one. For that reason it is expected to receive many millions of exceptions all saying Invalid password (or invalid Network).

Agree

Please have a look here: https://github.com/MetacoSA/NBitcoin/blob/a9b8e592fa7df0e97bdffe7de94ed58a87d92144/NBitcoin/BIP38/BitcoinEncryptedSecret.cs#L211. That NBitcoin's GetKey method is designed in a way that SecurityException is thrown when the provided password is incorrect.

Agree

This is the normal usage of password finder. In case this code is used for any RPC in future or attackers could find other ways to exploit application, it can be used in an unexpected way. So, its a best practice to avoid writing code with blank catch blocks.

More context:

https://stackoverflow.com/questions/1234343/why-are-empty-catch-blocks-a-bad-idea

https://cwe.mitre.org/data/definitions/391.html

[kiminuo]

I think we can document better the intent of https://github.com/zkSNACKs/WalletWasabi/pull/6791/files#diff-882482818afddc9c3a58bb9a865609886c5a6c4a322bd781233e1d03a4dcf802R47 line.

Feel free to suggest a different idea how to make the code better.

[adamPetho]

nACK, this filles up the Logs file with Security exception: 'System.Security.SecurityException: Invalid password (or invalid Network), just like Lucas said.

Maybe Logger.LogTrace("...") is enough here? I'm not sure.

[kiminuo]

Maybe Logger.LogTrace("...") is enough here? I'm not sure.

IMO no. It just doesn't make too much sense. The exception is not really exceptional state here. It simply says that "the password is not correct" but that is to be expected here.

[ghost]

nACK, this filles up the Logs file with Security exception: 'System.Security.SecurityException: Invalid password (or invalid Network), just like Lucas said.

So few lines in log file about wrong password is an issue? But an attacker exploiting the application is not an issue?

[ghost]

A similar pull request. #6648

[kiminuo]

So few lines in log file about wrong password is an issue? But an attacker exploiting the application is not an issue?

If the log line has its merit, we can do something about it. The problem is that I don't get what you have in mind. Can you explain in simple terms how one can exploit the code or how that log line helps in not-exploiting the code?

I have explained here #6791 (comment) how the try catch came to be. You can also create a PR on NBitcoin's side with my proposed API. That seems like the correct thing to do here to me.

[ghost]

Can you explain in simple terms how one can exploit the code or how that log line helps in not-exploiting the code?

I can't believe I am arguing why we should not have empty catch block. You can read the links I shared and it's a best practice to avoid unnecessary things in future. If I could find a way to exploit it right now that would be a vulnerability in Wasabi and qualify for CVE which won't be discussed here publicly.

Adding two lines, improving the code and security makes more sense IMO. These few lines would certainly not make anything worse. If someone is trying wrong password thousands of times, should not have issues with log file. Its the same with Bitcoin Core.

[kiminuo]

Can you explain in simple terms how one can exploit the code or how that log line helps in not-exploiting the code?

I can't believe I am arguing why we should not have empty catch block. You can read the links I shared and it's a best practice to avoid unnecessary things in future. If I could find a way to exploit it right now that would be a vulnerability in Wasabi and qualify for CVE which won't be discussed here publicly.

Ok, so there are few things to make clear:

• NBitcoin is considered to be a safe library currently by wasabi team. This simply means that we trust it as we trust .NET. So if somebody tampers with NBitcoin, then yes, we may be in trouble because this assumption is used everywhere in the codebase I think.
• Adding a logging line at that place will lead to, based on configuration, to very bad performance because logger waits for filesystem to write the log line. So if the logger is turned on, op/s will be much lower.
• I don't understand why you don't want to fix the issue on NBitcoin's side and rather change the code here.

I can't believe I am arguing why we should not have empty catch block.

If you just present one or two scenarios how you would actually use that to do something bad, your argument here will be much stronger. Please try to understand that your expertise and expertise of others differ.

[molnard]

@prayank23 what's up with this, abandoned?

[ghost]

@prayank23 what's up with this, abandoned?

It should have been merged. I don't understand the arguments for not following best practice and fix an empty catch block.

[kiminuo]

@prayank23 what's up with this, abandoned?

It should have been merged. I don't understand the arguments for not following best practice and fix an empty catch block.

@prayank23 No, this is really unfair from you because:

• I described how the API works on NBitcoin's side: add LogCritical in empty catch block #6791 (comment)
• add LogCritical in empty catch block #6791 (comment) explains that this PR basically destroys the feature because you fill the log and performance of the feature will be abysmal.
• add LogCritical in empty catch block #6791 (comment) I asked you to provide an example how it can be exploited (with NBitcoin being trusted library). You haven't really (linking some stackoverflow site is not an explanation really)
• add LogCritical in empty catch block #6791 (comment) provides summary and recommends to fix it on NBitcoin's side.

And after all of that you recommend to merge this PR?

[ghost]

I described how the API works on NBitcoin's side: add LogCritical in empty catch block #6791 (comment)

NBitcoin can have some code it does not mean we would not follow basic secure coding practices in Wasabi.

explains that this PR basically destroys the feature because you fill the log and performance of the feature will be abysmal.

How many times do you think a normal user would use password finder? If it is used, do you think logs are an issue? if yes, why not have separate logs for it?

I asked you to provide an example how it can be exploited (with NBitcoin being trusted library). You haven't really (linking some stackoverflow site is not an explanation really)

Did you even open those links? There was a second link and you can google "Empty catch block security"

As far as example is concerned I shared how this could be abused/misused in ways that we can't predict right now. Maybe open wallet without password.

provides summary and recommends to fix it on NBitcoin's side.

NBitcoin can't be fixed for every small thing in this repository that we can't follow because logs are an issue. Maybe @NicolasDorier can also share his thoughts about empty-catch blocks.

And after all of that you recommend to merge this PR?

Yes because its related to security which is more important than anything else IMO

[ghost]

[lontivero]

NACK. There is not security issue here. This is not correct.

[yahiheb]

More context:

https://stackoverflow.com/questions/1234343/why-are-empty-catch-blocks-a-bad-idea

https://cwe.mitre.org/data/definitions/391.html

@prayank23 I am no expert on the matter and I am not here to rehash this topic again, but most (if not all) answers in the stackoverflow link say something like Usually empty try-catch is a bad idea because ... Occasionally this may be the right thing to do ...

So as Lucas and Kiminuo tried to say this is how that tool is designed, and it makes sense in this case to have that empty try-catch.

[ghost]

More context:
https://stackoverflow.com/questions/1234343/why-are-empty-catch-blocks-a-bad-idea
https://cwe.mitre.org/data/definitions/391.html

@prayank23 I am no expert on the matter and I am not here to rehash this topic again, but most (if not all) answers in the stackoverflow link say something like Usually empty try-catch is a bad idea because ... Occasionally this may be the right thing to do ...

So as Lucas and Kiminuo tried to say this is how that tool is designed, and it makes sense in this case to have that empty try-catch.

This got 58 upvotes in the same link so I guess at least one comment makes sense why nothing:

Although I would never use empty catch blocks in my projects

[yahiheb]

This got 58 upvotes in the same link so I guess at least one comment makes sense why nothing:

ACK