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

Security issue in FlyingSocks Socket.write #26

Closed
stackotter opened this issue Apr 7, 2022 · 2 comments
Closed

Security issue in FlyingSocks Socket.write #26

stackotter opened this issue Apr 7, 2022 · 2 comments

Comments

@stackotter
Copy link
Contributor

Overview

If the data passed to Socket.write is a slice with a non-zero startIndex, memory after the end of the data buffer will be leaked to the recipient.

Cause

The issue is on line 229 of Socket.swift:

let sent = try write(buffer.baseAddress! + index, length: data.endIndex - index)

The code assumes that buffer.baseAddress! + index correctly gets the byte at index in the data, however baseAddress points to the byte at startIndex not at index 0. For example, consider the following code:

let data = "abcd".data(using: .utf8)!
let slice = data[2...] // contains "cd"

let index = slice.startIndex
try slice.withUnsafeBytes { buffer in               
    _ = try write(buffer.baseAddress! + index, length: data.endIndex - index)
    //                   (1)            (2)            (3)
    //
    // 1. baseAddress points to "c"
    // 2. after adding startIndex (which is 2), the pointer points to the byte after the end of the buffer
    // 3. length is 2 (4 - 2)
    //
    // In this example scenario, the server accidentally sends two bytes of
    // the memory after the end of the data buffer to the client, which could
    // lead to sensitive data being leaked in certain setups. It could also potentially
    // be combined with certain other types of vulnerabilities to execute arbitrary code.
}

Proof of concept

First, run the following command to start a tcp listener that simply prints out any data it receives.

nc -l 8080

Next, run this code snippet with swift run for the highest chance of reproducing, because that's how I ran it:

@main
struct FlyingFoxDataTest {
    static func main() async throws {
        // Generate some dummy data and make a slice
        let data = String(repeating: "abcd", count: 32).data(using: .utf8)!
        let slice = data[64...]
        
        // This length of string seems to work consistently on my laptop
        let secretPassword = "thisismyverylongandsecuresecretpasswordthisismyverylongandsecuresecretpasswordthisismyverylongandsecuresecretpassworditissogood!".data(using: .utf8)!
        
        // Attempt to send the slice through the socket
        let socket = try await AsyncSocket.connected(to: .inet(ip4: "127.0.0.1", port: 8080), pool: .polling)
        try await socket.write(slice)
    }
}

When I run that snippet, I get the following output:

$ nc -l 8080
thisismyverylongandsecuresecretpasswordthisismyverylongandsecure

As you can see, it sent the first half of secretPassword instead of the contents of the data slice. This bug could have pretty bad side-effects if it appeared in any unfortunate situations.

Mitigation

Make the following change:

- let sent = try write(buffer.baseAddress! + index, length: data.endIndex - index)
+ let sent = try write(buffer.baseAddress! + index - data.startIndex, length: data.endIndex - index)

I'll make a PR to fix this soon.

And I know this full on bug report might be a bit overkill, but I had fun getting that proof of concept to work, so I did it anyway.

stackotter added a commit to stackotter/FlyingFox that referenced this issue Apr 7, 2022
The issue was caused by incorrect handling of data slices
with non-zero start indices and could lead to arbitrary
memory being leaked to the socket
swhitty added a commit that referenced this issue Apr 7, 2022
Fix security vulnerability in Socket.write (#26)
@swhitty
Copy link
Owner

swhitty commented Apr 7, 2022

Thank you for the great explanation and fix. I love learning about this stuff.

@swhitty swhitty closed this as completed Apr 7, 2022
@stackotter
Copy link
Contributor Author

You're welcome! I also enjoyed writing the explanation and creating the proof of concept, so it's a win win :)

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

2 participants