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

Return mutable versions instead of immutable versions. #19

Closed
wants to merge 1 commit into from

Conversation

tmeisenh
Copy link

@tmeisenh tmeisenh commented Oct 3, 2014

Changing return types from immutable to mutable versions. This gives consumers control/ownership over the data buffers used.

I was pulling data out of the keychain and not scrubbing it when finished and that creates an attack vulnerability from a security perspective (an audit flagged this). When the NSData was dealloc'd the underlying memory still contained the data. In order for my concern to be met I needed UIKCS to return me mutable versions so I could do what I needed to do.

@JRG-Developer
Copy link

👎 -1

This doesn't have any place within the main repository.

You shouldn't force library consumers to use mutable types.

If you have a requirement for your own project, fork the repo and use your own local version. Or even simpler, just call mutableCopy wherever you need a mutable type.

@tmeisenh
Copy link
Author

tmeisenh commented Oct 9, 2014

I appreciate the feedback JRG. This was my first pull request and I really half-assed the description of the change. I modified it to be more clear, I think.

The problem with mutableCopy is that it is a copy. If you call mutableCopy on say NSData you get an NSMutableData copy of the original data. If you manipulate the NSMutableData the original NSData remains untouched -- I still don't own the data.

My use case is probably an edge case but the current API was a bit restrictive and as far as I can tell there's no real reason the api is NSData as opposed to NSMutableData.

The underlying CFTypeRef's CFGetTypeId was the same id as both CFMutableDataRef and CFDataRef. My naive understanding of CF leads me to think at this level the types are interchangeable and I can just bridge cast to the mutable version and avoid an actual memory copy and get access to the actual original memory address. If this assumption is wrong someone please correct me :)

@dmiedema
Copy link

dmiedema commented Dec 1, 2014

@tmeisenh My gut reaction against returning NSMutableData instead of just an NSData object is that any changes made to the NSData object aren't going to be persisted to the keychain anyway so what difference does it make if its mutable or not; you still end up having to save your changes to the keychain explicitly. I can't quite see the benefit of mutable over immutable. To me, personally, immutable has more benefits than mutable (at least without an example case)

Its too late right now but come tomorrow (pending my memory not shitting the bed) i'll try this out and see if theres any actual implementation differences between the two.

@tmeisenh
Copy link
Author

tmeisenh commented Dec 3, 2014

@dmiedema that's a fair point. My use case was protecting the sensitive data; I didn't want to just dealloc the sensitive NSData because the contents of that NSData would actually still be in memory that was just free()'ed; memset_s to the rescue. If you had physical access to the device it would be possible to get the sensitive data (provided that no nothing that malloc'd or altered the memory).

@kishikawakatsumi
Copy link
Owner

UICKeyChainStore is being very widely used, can not merge breaking changes at this time.

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

Successfully merging this pull request may close these issues.

None yet

4 participants