Skip to content

[MFA-2038] Unsafe code removal. #112

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.

Sign up for GitHub

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

Merged
Artelas merged 1 commit into from
Apr 7, 2023
Merged

[MFA-2038] Unsafe code removal. #112

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
18 changes: 7 additions & 11 deletions Guardian/Generators/OneTimePasswordGenerator.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -82,17 +82,13 @@ struct OneTimePasswordGenerator: TOTP, HOTP {
var c = UInt64(counter).bigEndian
let buffer = Data(bytes: &c, count: MemoryLayout<UInt64>.size);
let digestData = hmac.sign(buffer)
let hash = digestData.withUnsafeBytes { (bytes: UnsafePointer<UInt8>) -> UInt32 in
let last = bytes.advanced(by: hmac.digestLength - 1)
let offset = last.pointee & 0x0f
let start = bytes.advanced(by: Int(offset))
let value = start.withMemoryRebound(to: UInt32.self, capacity: 1) { $0 }
var hash = UInt32(bigEndian: value.pointee)
hash &= 0x7fffffff
hash = hash % UInt32(pow(10, Float(self.parameters.digits)))
return hash
}

let length = MemoryLayout<UInt32>.size
// digestData.count - 1 will always be >0, because digestData depends on algorythm and can be only 20, 32, or 64.
let offset = Int(digestData[digestData.count - 1] & 0x0f)
// offset is always <=15, length is always 4, so prefix will always be within digestData count.
var hash = digestData.dropFirst(offset).prefix(length).reduce(0, { $0 << 8 | UInt32($1) })
Copy link
Contributor

@Widcket Widcket Apr 4, 2023

Choose a reason for hiding this comment

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

If I'm understanding correctly, what the original code did (from L85 to L89) was roughly the following:

Screenshot 2023-04-04 at 8 06 25 PM

Could you please expand how reducing the data (after excluding the offset) by bit-shifting to the left is equivalent to pointee of start.withMemoryRebound(to: UInt32.self, capacity: 1) { $0 }?

Copy link
Contributor Author

@Artelas Artelas Apr 5, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

let value = start.withMemoryRebound(to: UInt32.self, capacity: 1) { $0 }

Here we have start as pointer to array of UInt8. withMemoryRebound interpret this array as single UInt32 number. For example for array of 4 UInt8 [0x12, 0x34, 0x56, 0x78], withMemoryRebound will produce single UInt32: 0x78563412. Which is then converted by constructor UInt32(bigEndian:) to 0x1234567.

.reduce(0, { $0 << 8 | UInt32($1) })

Here on the first iteration we have UInt32 equal to 0. Which is 0x00000000. First we shift it by 8 bytex, which produces the same value and doing OR with first element which is 0x12. resulting in 0x00000012. On the next iteration we shift it by 8 bytes resulting in 0x00001200. After doing OR with the next element 0x34, the result is 0x00001234. By the end of iterations we have 0x12345678, which is equal to result of withMemoryRebound with bigEndian.

Copy link
Contributor

@Widcket Widcket Apr 5, 2023

Choose a reason for hiding this comment

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

Fantastic, thanks for the clarification!

Copy link
Contributor

@ionutmanolache-okta ionutmanolache-okta Apr 6, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

we shift it by 8 bytes
I assume you meant 8 bits

hash &= 0x7fffffff
hash = hash % UInt32(pow(10, Float(self.parameters.digits)))
return Int(hash)
}

Expand Down