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

Check socket address size #152

Merged
merged 4 commits into from
May 22, 2024
Merged

Conversation

Cravtos
Copy link
Contributor

@Cravtos Cravtos commented May 15, 2024

Fixes panic when malformed socket address is parsed (e.g. during fuzz testing).

Copy link

cla-checker-service bot commented May 15, 2024

💚 CLA has been signed

@andrewkroh
Copy link
Member

during fuzz testing

It sounds like we should add that fuzz test into auparse/sockaddr_test.go. WDYT @Cravtos?

Copy link
Member

@andrewkroh andrewkroh left a comment

Choose a reason for hiding this comment

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

Can you please add an entry into the changelog at:

### Changed

The code changes LGTM.

@Cravtos
Copy link
Contributor Author

Cravtos commented May 16, 2024

Added fuzz test and updated changelog: b5915b8.

@Cravtos Cravtos requested a review from andrewkroh May 19, 2024 08:22
andrewkroh
andrewkroh previously approved these changes May 20, 2024
@andrewkroh
Copy link
Member

/test

@andrewkroh andrewkroh requested review from a team May 20, 2024 20:04
@andrewkroh andrewkroh added Team:Security-Linux Platform Linux Platform Team in Security Solution Team:Security-Service Integrations Security Service Integrations Team labels May 20, 2024
andrewkroh
andrewkroh previously approved these changes May 20, 2024
Copy link
Member

@andrewkroh andrewkroh left a comment

Choose a reason for hiding this comment

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

LGTM, but needs an additional reviewer because I pushed a gofumpt change.

efd6
efd6 previously approved these changes May 20, 2024
Copy link
Contributor

@efd6 efd6 left a comment

Choose a reason for hiding this comment

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

but nit.

"strconv"
)

// errInvalidSockAddrSize means socket address size is invalid.
var errInvalidSockAddrSize = errors.New("invalid socket address size")
Copy link
Contributor

Choose a reason for hiding this comment

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

It would be nice if the errors returned included the family and the len of the provided address.

type invalidSockAddrErr struct {
    family string
    size   int
}

func (e invalidSockAddrErr) Error() string {
    return fmt.Sprintf("invalid socket address size family=%s: %d, e.family, e.size)
}

populated appropriately below.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

What family to specify in case of len(sockAddr) < 4? Or it would be better to introduce another error type for malformed families? Like so:

type invalidSockFamilyError struct {
    family string
}

func (e invalidSockFamilyError) Error() string {
    return fmt.Sprintf("invalid socket address family=%s", e.family)
}

// ...

if len(s) < 4 {
    return nil, invalidSockFamilyError{
        family: s,
    }
}

Copy link
Contributor

Choose a reason for hiding this comment

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

Suggest either calling the family "invalid", or conditionally format an error in the Error method when the size is less than 4 and don't use the family field in that case.

type invalidSockAddrErr struct {
    family string
    size   int
}

func (e invalidSockAddrErr) Error() string {
    if e.size < 4 {
        return fmt.Sprintf("invalid family: too short: %d", e.size)
    }
    return fmt.Sprintf("invalid socket address size family=%s: %d, e.family, e.size)
}

and in the preamble of the func,

func parseSockaddr(s string) (map[string]string, error) {
	if len(s) < 4 {
		return nil, invalidSockAddrErr{size: len(s)}
	}
...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done: b1c5994

@Cravtos Cravtos dismissed stale reviews from efd6 and andrewkroh via b1c5994 May 21, 2024 08:41
@efd6
Copy link
Contributor

efd6 commented May 22, 2024

/test

@elasticmachine
Copy link
Collaborator

💚 Build Succeeded

History

Copy link
Contributor

@efd6 efd6 left a comment

Choose a reason for hiding this comment

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

LGTM but waiting for @andrewkroh

Copy link
Member

@andrewkroh andrewkroh left a comment

Choose a reason for hiding this comment

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

LGTM. Thank you for the contribution.

@andrewkroh andrewkroh merged commit e1703ad into elastic:main May 22, 2024
3 checks passed
@Cravtos Cravtos deleted the check-sockaddr-size branch May 22, 2024 07:03
renini pushed a commit to renini/go-libaudit that referenced this pull request Jun 26, 2024
In auparse.parseSockaddr(), check socket address size
before indexing into the slice to prevent panics. Return an
error detailing the failure.

Add a Go fuzz test to exercise the parseSockaddr() function.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Team:Security-Linux Platform Linux Platform Team in Security Solution Team:Security-Service Integrations Security Service Integrations Team
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants