Fix: Dynamic Stack Buffer Overflow

[chosa91]

Given
FlyingFox: 0.14.0
Device: iPhone 15 iOS 17.5 (Simulator)

When
Turn on AddressSanitizer in Xcode.

Then
I've encountered a dynamic stack buffer overflow error.

Address 0x00016cead4bc is located in stack of thread T0
SUMMARY: AddressSanitizer: dynamic-stack-buffer-overflow (/Users/__WHOAMI__/Library/Developer/CoreSimulator/Devices/95E3F30B-6F79-410F-8A4C-68EC8706820E/data/Containers/Bundle/Application/560390B8-52F3-4465-9BD6-82AA4A9EB557/__APP__.app/__APP__:arm64+0x103471ea4) in closure #1 (Swift.UnsafePointer<__C.sockaddr_storage>) -> __C.sockaddr_storage in closure #1 (Swift.UnsafePointer<A>) -> __C.sockaddr_storage in (extension in FlyingSocks):FlyingSocks.SocketAddress.makeStorage() -> __C.sockaddr_storage+0x9b0
Shadow bytes around the buggy address:
  0x00016cead200: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
  0x00016cead280: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
  0x00016cead300: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
  0x00016cead380: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
  0x00016cead400: 00 00 00 00 ca ca ca ca 00 00 00 00 cb cb cb cb
=>0x00016cead480: ca ca ca ca 00 00 00[04]cb cb cb cb 00 00 00 00
  0x00016cead500: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
  0x00016cead580: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
  0x00016cead600: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
  0x00016cead680: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
  0x00016cead700: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
Shadow byte legend (one shadow byte represents 8 application bytes):
  Addressable:           00
  Partially addressable: 01 02 03 04 05 06 07 
  Heap left redzone:       fa
  Freed heap region:       fd
  Stack left redzone:      f1
  Stack mid redzone:       f2
  Stack right redzone:     f3
  Stack after return:      f5
  Stack use after scope:   f8
  Global redzone:          f9
  Global init order:       f6
  Poisoned by user:        f7
  Container overflow:      fc
  Array cookie:            ac
  Intra object redzone:    bb
  ASan internal:           fe
  Left alloca redzone:     ca
  Right alloca redzone:    cb
==30279==ABORTING

Source of error:

FlyingFox/FlyingSocks/Sources/SocketAddress.swift

Lines 118 to 126 in 3d58c54

	func makeStorage() -> sockaddr_storage {
	var addr = self
	return withUnsafePointer(to: &addr) {
	$0.withMemoryRebound(to: sockaddr_storage.self, capacity: 1) {
	$0.pointee
	}
	}
	}
	}

This is probably due to the program writes more data to a buffer located on the stack than it was allocated for, leading to memory corruption.

The makeStorage() function is attempting to cast a socket address (sockaddr_in, sockaddr_in6, or sockaddr_un) to a sockaddr_storage object. The issue arises when the memory layout of sockaddr_storage does not align with the original structure's layout, causing an overflow.

[swhitty]

Thanks for reporting this @chosa91 🙏🏻.

sockaddr_storage is only really used as box for any address, I think it might be simpler to instead remove makeStorage() completely and just use any SocketAddress. SE-0352 implicitly unboxes the address when required — I've pushed the branch remove-socket_storage that does this and it appears to correct the sanitizer issues. What do you think?

That said, I am keen to understand the best way to copy sockaddr* into sockaddr_storage as it is used in unit tests, So I am interested in your implementation for the tests here.

[swhitty]

Do you think the existing tests within SocketAddressTests already cover these same things?

• testAddress_DecodesIP4()
• testINET_ThrowsInvalidAddress_WhenFamilyIncorrect()
• testAddress_DecodesIP6()
• testINET6_ThrowsInvalidAddress_WhenFamilyIncorrect()
• testUnix_IsCorrectlyDecodedFromStorage()
• testUnix_ThrowsInvalidAddress_WhenFamilyIncorrect()

The existing makeStorage() is tested within the "decode" tests above. They are more of an encode/decode — they start with an address, call makeStorage() then convert back to the original address to assert the values are the same.

[chosa91]

Since they didn't run red, they should definitely be modified, if you think I'd better merge the tests. Since they run quite quickly, I thought it was no problem to validate the storage specific ones separately.

Tastes and slaps, I don't like overlapping test cases, however as our example shows, they can mask underlying-unit errors.

[chosa91]

Also, if we go along with the current solution, I would like to note that I have worded the tests differently, if you like, I would be happy to reword them to the existing style (with underscores).

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 97.77%. Comparing base (ea58a79) to head (40babf4).
Report is 1 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #109      +/-   ##
==========================================
- Coverage   98.00%   97.77%   -0.24%     
==========================================
  Files          55       55              
  Lines        2460     2467       +7     
==========================================
+ Hits         2411     2412       +1     
- Misses         49       55       +6     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[chosa91]

Thanks for reporting this @chosa91 🙏🏻.

sockaddr_storage is only really used as box for any address, I think it might be simpler to instead remove makeStorage() completely and just use any SocketAddress. SE-0352 implicitly unboxes the address when required — I've pushed the branch remove-socket_storage that does this and it appears to correct the sanitizer issues. What do you think?

That said, I am keen to understand the best way to copy sockaddr* into sockaddr_storage as it is used in unit tests, So I am interested in your implementation for the tests here.

Sorry, I've missed this comment. It makes sense to get to the root of the problem. Feel free to use the content of my PR and close this.

[swhitty]

Thank you, I'll merge this PR then play my any SocketAddress changes over the top moving your makeStorage() implementation to the unit tests