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

Add implementation and tests for stpncpy() security wrapper #87

Merged
merged 2 commits into from
May 31, 2024

Conversation

fhgwright
Copy link
Contributor

See the two commit messages.

Tested on:

Mac OS X 10.4.11 8S165, PPC, Xcode 2.5 8M2558
Mac OS X 10.4.11 8S2167, i386, Xcode 2.5 8M2558
Mac OS X 10.5.8 9L31a, PPC, Xcode 3.1.4 9M2809
Mac OS X 10.5.8 9L31a, i386, Xcode 3.1.4 9M2809
Mac OS X 10.5.8 9L31a, x86_64, Xcode 3.1.4 9M2809
Mac OS X 10.5.8 9L31a, PPC (i386 Rosetta), Xcode 3.1.4 9M2809
Mac OS X 10.6.8 10K549, i386, Xcode 3.2.6 10M2518
Mac OS X 10.6.8 10K549, x86_64, Xcode 3.2.6 10M2518
Mac OS X 10.6.8 10K549, PPC (i386 Rosetta), Xcode 3.2.6 10M2518
Mac OS X 10.7.5 11G63, x86_64, Xcode 4.6.3 4H1503
OS X 10.8.5 12F2560, x86_64, Xcode 5.1.1 5B1008
OS X 10.9.5 13F1911, x86_64, Xcode 6.2 6C131e
OS X 10.10.5 14F2511, x86_64, Xcode 7.2 7C68
OS X 10.11.6 15G22010, x86_64, Xcode 8.1 8B62
macOS 10.12.6 16G2136, x86_64, Xcode 9.2 9C40b
macOS 10.13.6 17G14042, x86_64, Xcode 10.1 10B61
macOS 10.14.6 18G9323, x86_64, Xcode 11.3.1 11C505
macOS 10.15.7 19H15, x86_64, Xcode 12.4 12D4e
macOS 11.7.10 20G1427, x86_64, Xcode 13.2.1 13C100
macOS 11.7.10 20G1427, arm64, Xcode 13.2.1 13C100
macOS 12.7.5 21H1222, x86_64, Xcode 14.2 14C18
macOS 12.7.5 21H1222, arm64, Xcode 14.2 14C18
macOS 13.6.7 22G720, arm64, Xcode 15.2 15C500b
macOS 14.5 23F79, arm64, Xcode 15.4 15F31d

This adds support for the optional stpncpy() security wrapper, based
on the _FORTIFY_SOURCE setting.  By default, it only impacts 10.6
builds, though it can be explicitly enabled on 10.5, albeit less
efficiently due to the lack of the compiler builtin.

The stpncpy() function is the only one with an optional security
wrapper which is also optionally provided by legacy-support.  Hence,
this is the only addition needed to close the more general ticket.

Closes: https://trac.macports.org/ticket/69878

Also fixes a minor comment formatting issue.

TESTED:
Tested on 10.4-10.5 ppc, 10.4-10.6 i386, 10.5-10.6 ppc (i386 Rosetta),
10.5-12.x x86_64, 11.x-14.x arm64.
Passes all tests, including newly added tests for this feature.
@fhgwright
Copy link
Contributor Author

@mascguy
Adding notification manually.

@kencu
Copy link
Contributor

kencu commented May 27, 2024

Getting closer now…

but just put it in the expected header.

@kencu
Copy link
Contributor

kencu commented May 27, 2024

And….why all the previous drama and arguing?

@fhgwright
Copy link
Contributor Author

@kencu

Getting closer now…

but just put it in the expected header.

If by "expected header" you mean an LS secure/_string.h (currently nonexistent), well, let's consider that:

First of all, a header of that name would shadow the SDK copy, so it would need to use #include_next to get around that. So far, so good. But it would be applied as part of the #include_next <string.h in the LS string.h, and therefore before the declaration of stpncpy(). That would reintroduce the same old build failure, but now not limited to the SDK mismatch case. So it would need to play some games with the guard macros and get included a second time at the bottom of LS string.h, in order to happen at the right time.

The above could be simplified by changing the name of the directory and/or file so that it didn't collide with the SDK secure/_string.h. But then it's no longer exactly mimicking the SDK layout, so it just boils down to the question of whether it makes sense to split string.h into two files or keep it as one. It's logically a single header in either case, since including string.h implies including the secondary header (in the relevant cases), and not only is including the secondary header directly explicitly documented as illegal, but also there's even code to generate an error if you try. The only thing that matters to the end user is whether #include <string.h> does the right thing for the chosen (or defaulted) value of _FORTIFY_SOURCE.

The only plausible explanation for why the normal practice is to split the header into two parts is the reduction of clutter in the main header (particularly since the whole security-wrapper thing was an afterthought). But that's actually a mixed blessing, since it splits the information regarding a given function into two separate pieces in two separate files, thus obfuscating the full picture. It's further obfuscated by the fact that the #include <secure/_string.h> happens at the bottom of string.h, which is not where headers including other headers usually happens. Doing it last is of course a necessity in this case in order to do things in the correct order, but nevertheless, having an additional #include in an unusual place adds to the obfuscation.

Also note that this whole issue in the LS context is for a grand total of one function, making the "clutter" fairly minimal. So the question is whether to take the simple and more understandable approach of putting everything in one header, or taking the more complicated and more obtuse approach of splitting it in two, just for the questionable goal of kinda sorta mimicking the SDK header layout.

Verdict: One file.

It might surprise you to learn that I thought about all of this before implementing anything. And just that alone took well over "five minutes". :-) As did explaining it.

BTW, Apple added the source code for stpcpy() and stpncpy() to their codebase at the same time, at least at the commit resolution of their published codebase. So they apparently just forgot to "publish" stpncpy() until 10.7.

@kencu

And….why all the previous drama and arguing?

Because implementing, debugging, and testing the enhancement, plus implementing, debugging, and testing the test for the enhancement was massively more effort than the one-line addition sufficient to fix the actual bug. That fix is already deployed and working.

The whole "five minutes" thing was clearly based on a variant of Weiler's law.

@mascguy
Aren't you glad now that you didn't implement stpncpy()? :-)

test/test_stpncpy_chk.c Outdated Show resolved Hide resolved
Copy link
Member

@mascguy mascguy left a comment

Choose a reason for hiding this comment

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

Apart from my one question, this looks great! Everything is well-documented, logically organized, etc.

And while I haven't tested with it yet, there are no concerns here.

Anyone else? @kencu and @MarcusCalhoun-Lopez ?

This adds three tests for the proper behavior of the stpncpy()
security wrapper, both with and without explictly setting
_FORTIFY_SOURCE.

TESTED:
Tested on 10.4-10.5 ppc, 10.4-10.6 i386, 10.5-10.6 ppc (i386 Rosetta),
10.5-12.x x86_64, 11.x-14.x arm64.
Fails in expected cases without the corresponding fix, and passes in
all cases with the fix.
@fhgwright
Copy link
Contributor Author

@mascguy
Ping.

I have other changes coming soon, so it may not make sense to be in a hurry to update the -devel port.

@mascguy
Copy link
Member

mascguy commented May 31, 2024

I have other changes coming soon, so it may not make sense to be in a hurry to update the -devel port.

I'm fine with either merging ASAP, or deferring until your other changes are done. Up to you!

@fhgwright
Copy link
Contributor Author

@mascguy

I have other changes coming soon, so it may not make sense to be in a hurry to update the -devel port.

I'm fine with either merging ASAP, or deferring until your other changes are done. Up to you!

Those two things are separate. I'd like to get this PR merged, since there are lexical conflicts with other things I'm working on, but there's no rush for a companion update to the -devel port.

Unless someone is really champing at the bit to get security wrappers for C-only uses of stpncpy() on 10.6. :-)

@mascguy
Copy link
Member

mascguy commented May 31, 2024

Those two things are separate. I'd like to get this PR merged, since there are lexical conflicts with other things I'm working on, but there's no rush for a companion update to the -devel port.

Got it, I'll go ahead and merge then. Thanks for clarifying!

@mascguy mascguy merged commit 61be35a into macports:master May 31, 2024
@fhgwright fhgwright deleted the stpncpy branch May 31, 2024 22:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

3 participants