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

#736 Generate hash and sign/verify data for secrettext values #1005

Closed
wants to merge 5 commits into from
Closed

#736 Generate hash and sign/verify data for secrettext values #1005

wants to merge 5 commits into from

Conversation

TKapitan
Copy link
Contributor

@TKapitan TKapitan commented Apr 27, 2024

Summary

New overloaded procedures for Hash and Sign/Verify that accept SecretText value as hash/sign input value.

Work Item(s)

Fixes #736

Fixes AB#507370

@TKapitan TKapitan requested a review from a team as a code owner April 27, 2024 07:18
@github-actions github-actions bot added AL: System Application From Fork Pull request is coming from a fork labels Apr 27, 2024
@github-actions github-actions bot added this to the Version 25.0 milestone Apr 27, 2024
Copy link
Contributor

@darjoo darjoo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Other than the change of where the unwrapping happens, looks fine.

@JesperSchulz JesperSchulz added the Integration GitHub request for Integration area label Apr 30, 2024
@aholstrup1 aholstrup1 added the Linked Issue is linked to a Azure Boards work item label Apr 30, 2024
@BazookaMusic
Copy link
Contributor

BazookaMusic commented Apr 30, 2024

@darjoo , @WaelAbuSeada
I'm not sure it's a good idea to let this go in. This can allow
an attacker to brute force the contents of a secrettext.

The way they would do it is to simply write a brute force search comparing the secret text with different combinations and then adding a breakpoint inside an if statement chrcking if the password matches.

@darjoo
Copy link
Contributor

darjoo commented May 1, 2024

@darjoo , @WaelAbuSeada I'm not sure it's a good idea to let this go in. This can allow an attacker to brute force the contents of a secrettext.

The way they would do it is to simply write a brute force search comparing the secret text with different combinations and then adding a breakpoint inside an if statement chrcking if the password matches.

@TKaptian Could you let us know the scenarios you have that require this? What type of values are in the secrettext and why that needs to be hashed?

@JesperSchulz JesperSchulz added the Follow Up The issue has an open question and must be followed up on label May 13, 2024
@pri-kise
Copy link
Contributor

pri-kise commented May 27, 2024

@darjoo there's an thread on yammer about this topic:
https://www.yammer.com/dynamicsnavdev/threads/2808751498059776

I think one good example is to use this for Basic Authentication that requires a Base64 hash.
e.g. if you want to implement something similiar to the procedure ToBase64(String: SecretText) Base64String: SecretText here:

local procedure ToBase64(String: SecretText) Base64String: SecretText

I know that there exists better authentication methods than base64 but sometimes we aren't in the control of the external service and the external service only provides simple authentication methods.

And here's another example where such a hash procedure would be used: To Compare the hash of passwords
https://www.yammer.com/dynamicsnavdev/threads/2841855068626944

@TKapitan TKapitan requested review from WaelAbuSeada and darjoo June 21, 2024 11:11
@JesperSchulz
Copy link
Contributor

@darjoo, when time allows, could you peek into this one?

@darjoo
Copy link
Contributor

darjoo commented Jul 5, 2024

@BazookaMusic @pri-kise Has valid scenarios

  1. Basic Auth
  2. Password comparisons (From yammer)

We had a discussion about 1., and the idea is to provide platform functionality to do that instead of App. However, that does not resolve scenario 2.

So, do you have any thoughts on that? Because if we do not allow SecretText to Text or SecretText == SecretText, our partners would not be able to do password comparisons. That would bring up your concerns of being able to bruteforce the SecretText and that wouldn't be addressed by the platform functionality of no. 1.

@TKapitan
Copy link
Contributor Author

@BazookaMusic @pri-kise @darjoo @JesperSchulz @WaelAbuSeada I'm not a security expert, but in my opinion, the idea of "cracking" the SecretText using brute force here makes no sense. Why would someone do such a complicated thing if there are other, much easier solutions?

The secret text is useful for hiding values during debugging. But it does not protect values from other developers, at least not if you make the variable somewhere accessible (event, etc.).

Why? Because what prevents any developer just to add any SecretText variable to HttpHeaders and send it to mock server? This will be much easier way if someone wants to "decrept" the secrettext variable... And that's something you can't prevent if you want to make the secrettext variable useful (because the only other solution is to not allow usage of secrettext when interacting with third party/other Microsoft systems.

SecretText is a good replacement for [NonDebuggable], but that's about it. If you want to avoid exposing something, do not make it accessible.

Based on what I said above, I don't see any reasons why we should not be able to generate hashed values for secrettexts, as there are already easier way how to "crack" the secrettext value.

@BazookaMusic
Copy link
Contributor

@BazookaMusic @pri-kise @darjoo @JesperSchulz @WaelAbuSeada I'm not a security expert, but in my opinion, the idea of "cracking" the SecretText using brute force here makes no sense. Why would someone do such a complicated thing if there are other, much easier solutions?

The secret text is useful for hiding values during debugging. But it does not protect values from other developers, at least not if you make the variable somewhere accessible (event, etc.).

Why? Because what prevents any developer just to add any SecretText variable to HttpHeaders and send it to mock server? This will be much easier way if someone wants to "decrept" the secrettext variable... And that's something you can't prevent if you want to make the secrettext variable useful (because the only other solution is to not allow usage of secrettext when interacting with third party/other Microsoft systems.

SecretText is a good replacement for [NonDebuggable], but that's about it. If you want to avoid exposing something, do not make it accessible.

Based on what I said above, I don't see any reasons why we should not be able to generate hashed values for secrettexts, as there are already easier way how to "crack" the secrettext value.

I agree with the first part which is that we cannot protect the SecretText value from being extracted from someone installing an extension to brute force it by installing an extension and subscribing an event or something similar. The easiest way to reveal it is as you said to send it via http request somewhere. There is some logging on this to detect it but is besides the point. However, I shouldn't have focused on this scenario.

The actual problem is if the hashed value of a SecretText should be a Text or a SecretText. The purpose of the type as far as credentials go is to prevent somebody from revealing them by debugging them. This can be a delegated admin who should not have direct access to customer credentials like a password.

The issue I see is that hashed passwords should also not be revealed to a delegated admin. A hashed password is still an important attack vector, especially if passwords are user generated. The simplest example is that an admin debugging through an extension's code hovers over a hashed user password and notes it down. Then they use a dictionary attack to try passwords on it and get the user credential. This action would not be logged, since we can't log every time someone hovers over a text variable.

Thus, I think it's fine to make a hash out of a SecretText and to compare the hashes, but I don't think it's fine to allow the hash to be in plain-text.

@TKapitan
Copy link
Contributor Author

@BazookaMusic @pri-kise @darjoo @JesperSchulz @WaelAbuSeada Thanks for your input. I have changed the procedures to return SecretText instead.

@TKapitan
Copy link
Contributor Author

On the other hand, I'm not thinking if this is now useful... We don't have (for good reason) how to compare secret texts. So is it now even useful? Because we would need the ability to compare hashes, otherwise it makes no sense to hash value...

@BazookaMusic @pri-kise @darjoo @JesperSchulz @WaelAbuSeada

@TKapitan TKapitan closed this by deleting the head repository Aug 21, 2024
@TKapitan
Copy link
Contributor Author

Upps, my fork cleanup was a bit disaster... I'll try to restore this PR in the upcoming days.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
AL: System Application Follow Up The issue has an open question and must be followed up on From Fork Pull request is coming from a fork Integration GitHub request for Integration area Linked Issue is linked to a Azure Boards work item
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[BC Idea]: GenerateHash() with InputString: SecretText
7 participants