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

bitlockerConversionStatus broken by type change #43

Open
ItsMattL opened this issue Mar 29, 2022 · 5 comments
Open

bitlockerConversionStatus broken by type change #43

ItsMattL opened this issue Mar 29, 2022 · 5 comments

Comments

@ItsMattL
Copy link
Contributor

It looks like commit f838a77 inadvertently broke the bitlockerConversionStatus by changing the types from int32 to uint32. Our dependent code started returning the "unable to get conversion status while getting BitLocker info; the volume is locked" error. Further investigation suggested this was due to !ok triggering on the type conversion, rather than on the value itself.

It should be possible to confirm this by checking the behavior with reflect.TypeOf:
fmt.Printf("Result type: %s", reflect.TypeOf(statusResultRaw.Value()))

@iamacarpet
Copy link
Owner

Apologies @ItsMattL ,

I hadn’t spotted that in review, it was a poor review on my part and we haven’t updated to the tip of master for a while.

You may well be a more active user than I am myself so I’d like your opinion: would you class anything in that commit as useful, or would you revert the whole thing?

@ItsMattL
Copy link
Contributor Author

Yeah no worries. The constants certainly seem useful. I'll also say I haven't validated the types on all of the values to see if uint is correct for any of them, so ideally someone else could do a verification on the behavior here and make sure it's not something weird in our version of ole or something. The Microsoft docs are only so reliable in my experience, at least when being translated through go-ole. Sometimes the types match exactly, other times you get these weird mismatches. I'm curious if this actually worked for the original contributor, or if it was entirely based on the API docs.

@iamacarpet
Copy link
Owner

Ditto on the Microsoft docs not matching what types go-ole seems to return, took a lot of trial an error when first writing this stuff.

I’ll be honest and say that at work, I’m too tied up with other projects and will likely only get a few days later in the year to properly do anything related to this, so I can’t fully commit the time to resolving this at the minute, as I think you are right that it’ll need some proper testing & validation if we’re going to change the types.

I’ll revert the commit when I’m back in the office so the tip of master is back to something that had been stable for a fairly long time.

In the meantime, @safinsingh , are you interested in joining the conversation and possibly fixing your contribution?

@safinsingh
Copy link
Contributor

I believe I mentioned in my pull request that the return value should be an unsigned integer according to these Microsoft docs: https://docs.microsoft.com/en-us/windows/win32/secprov/getconversionstatus-win32-encryptablevolume. I would make sure there is no issue with the environment and that statusResultRaw.Value() is not returning 0x80310000 before reverting the types. It could very well be some sort of issue with the docs themselves because they haven't been entirely reliable in my experience as well. I just want to ensure utmost correctness of the types which is why I would want to make sure that the issue is not with your environment first.

@iamacarpet
Copy link
Owner

Just had a bit more time to look into this and I was wrong, we are using a version of master that has this included.

I believe we are just using a different function that wasn't touched by this change, GetBitLockerRecoveryInfo.

I can see in the history, there was one change where I swapped some values to uint32 myself:
0fa5b6b
Then swapped it back again as it wasn't working (but only in some situations):
315d753

I'm going from memory here, but we don't really touch this code unless something breaks in our internal app & we have to spend time on it, so I believe we started getting weird behaviour in a particular version of Windows 10 (we use Windows 10 x64 exclusively), hence the change to uint32 which initially fixed it.

Another Windows 10 update seemed to change things back to needing int32 instead, but we had to support both versions simultaneously (any that's why the second commit didn't just change the type inference back).

I never touched the code you are both referencing as I don't believe we are using it in our app, but perhaps it needs the same kind of check to support both?

I agree that it would be really helpful to confirm if you are both seeing inconsistent behaviour:

  • What types are being returned?
  • What versions of Windows are you using? Full build numbers ideally.
  • What version of go-ole are you both using? The tip of master appears to have it fixed in go.mod to github.com/go-ole/go-ole v1.2.6, can you confirm if this is what your local project is following?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants