cunicu / go-rosenpass Goto Github PK

By @koraa in #27 (comment)

https://github.com/stv0g/go-rosenpass/blob/d7e38ecaf9e7803f2824a03ac24ac34944a53af6/biscuit.go#L8

Severity: A (Functionality) – Biscuit counter is 12 bytes in size; using just 8 bytes is not necessarily catastrophic. The biscuit counter is always created and consumed by the same peer, so changing the format is not catastrophic; however it is a violation of the spec. Using just 8 bytes may also have an impact on overflow handling which I am unable to assess right now

See: open-quantum-safe/liboqs-go#25

• Rust (initiator) <-> Rust (responder)
• Golang (initiator) <-> Golang (responder)
• Rust (initiator) <-> Golang (responder)
• Golang (initiator) <-> Rust (responder)

By @koraa in #27 (comment)

https://github.com/stv0g/go-rosenpass/blob/d7e38ecaf9e7803f2824a03ac24ac34944a53af6/cmd/gen_config.go#L26

Severity: 3 (Dangerous) – Public key hard-coded in source

By @koraa in #27 (comment)

https://github.com/stv0g/go-rosenpass/blob/d7e38ecaf9e7803f2824a03ac24ac34944a53af6/cmd/main.go#L104

Severity: A (Functionality) – Path to man pages hard coded (will be incompatible with installing as a system package)

Thank you for writing this module. I think you did an incredible job with this!
Cryptography implementation is hard; I will document any observations about crypto I make

I will use the following scale to mark my observations:

1. Critical
2. Severe
3. Dangerous

--------

4. Concerning
5. Tradeoff
6. (Just barely) acceptable

--------

A. Functionality
B. Observation

The first block of ratings is for vulnerabilities with a high impact; these must be addressed, otherwise I would strongly argue against anyone ever using go-rosenpass. The second block is for issues that may reduce the security of go-rosenpass but not to the point that I would advise against using go-rosenpass.

EDIT: The last category is about the quality of the code, not about the security.

Summary

• #40
• #41
• #42
• #43
• #44
• #45
• #46
• #47
• #48
• #49
• #50
• #51
• #52
• #56
• #53
• #55
• #54

The current implementation only performs a single handshake

By @koraa in #27 (comment)

https://github.com/stv0g/go-rosenpass/blob/d7e38ecaf9e7803f2824a03ac24ac34944a53af6/handshake_initiator.go#L101

https://github.com/stv0g/go-rosenpass/blob/d7e38ecaf9e7803f2824a03ac24ac34944a53af6/server.go#L143

Severity: 3 (Dangerous) – No IO should be performed in response to maliciously crafted messages as performing IO opens up pandoras box in regards to DOS attacks (think of logs filling up, disk exhaustion, ghastly stuff like that).

Note that this performs a lot of allocations even if log level Debug was used.

By @koraa in #27 (comment)

Severity: 3 (Dangerous): No zeroization is attempted.

golang/go#21865

By @koraa in #27 (comment)

https://github.com/stv0g/go-rosenpass/blob/d7e38ecaf9e7803f2824a03ac24ac34944a53af6/messages.go#L184-201

Severity: B – Manual offset arithmetic like that is treacherous. There is a danger of subtle errors.
In the Rust implementation we put a lot of work into writing a macro that automatically generates this code.
Would it be possible to do something similar in rust?

By @koraa in #27 (comment)

https://github.com/stv0g/go-rosenpass/blob/d7e38ecaf9e7803f2824a03ac24ac34944a53af6/biscuit.go#L8

Severity: 6 (Just barely acceptable) – Biscuit implementation is not side channel resistant. Increment and comparison operations might leak information through timing side channels

Cross-compiling with CGo is a pain as we also need to cross-compile liboqs for various target platforms.

Maybe we just cross-build without CGo and Cloudflares circl package?

Goreleaser also has some cross-compilation support including CGo: https://github.com/goreleaser/goreleaser-cross

By @koraa in #27 (comment)

https://github.com/stv0g/go-rosenpass/blob/d7e38ecaf9e7803f2824a03ac24ac34944a53af6/config/arguments.go#L83-L97

Severity: B (Observation) – This (entire block of code) is surprisingly manual code; isn't it possible to formulate that sort of logic automatically?

By @koraa in #27 (comment)

Severerity: N/A – What is the situation with regards to printf-like-injection attacks, shell injection attacks, log-injection attacks in go? I have seen some format strings.

as golang.org/x/exp/slog becomes part of the standard library

The current implementation does not rotate the biscuit key

By @koraa in #27 (comment)

https://github.com/stv0g/go-rosenpass/blob/d7e38ecaf9e7803f2824a03ac24ac34944a53af6/messages.go#L88

Severity: B – Can we please avoid using sentinel values like that and use something like the Rust Result type instead?
Can we use unsigned values? Where are unsigned values used in the application? Unsigned values run the danger of causing
subtraction where we expect addition if an attacker managed to inject them.

By @koraa in #27 (comment)

https://github.com/stv0g/go-rosenpass/blob/d7e38ecaf9e7803f2824a03ac24ac34944a53af6/config/peer_section.go#L87-L94

Severity: N/A – Please explain this code section.

By @koraa in #27 (comment)

Severerity: N/A – What is the situation with regards to switch-case-fallthrough and none of the branches of a switch statement being covered in go?

By @koraa in #27 (comment)

Severerity: N/A – Have you made sure to exclude the possibility of errors inside processing triggered by network messages that could crash the application?

By @koraa in #27 (comment)

https://github.com/stv0g/go-rosenpass/blob/d7e38ecaf9e7803f2824a03ac24ac34944a53af6/server.go#L200-L203

Severity: 2 (Severe) – At this point in the handshake the initiator is not authenticated.
This version of the code allows anyone to create a fake package and overwrite the correct peer address.
This boils down to a very check, remote disconnect attack.

We could introduce a schema for configuration files to automatically infer a Rosenpass configuration from an existing WireGuard config.

Example

• RP listen port = WG listen port (single port mode)
• RP peer endpoints = WG peer endpoints port (single port mode)
• RP static public key: /etc/wireguard/<intf-name>/pqpk
• RP static public key: /etc/wireguard/<intf-name>/pksk
• RP static peer public key: /etc/wireguard/<intf-name>/<peers-b64-wg-pubkey>.pqpk

https://pkg.go.dev/github.com/cloudflare/circl

Kyber-512 is supported. McEliece is under development:
cloudflare/circl#378

We currently treat handshake expiry and stale keys via the same timer.

https://github.com/awnumar/memguard

See: https://golangci-lint.run/usage/linters/#forbidigo

• unsafe
• fmt.Printf

The Rosenpass wire-protocol has been designed to be orthogonal to the WireGuard protocol.

As a result it is possible to speak both protocols over the same port which eases firewall / NAT configuration.

We can use the Linux/BSDs SO_ATTACH_FILTER socket option to attach a eBPF program which filters-out Rosenpass packets and leaves everything else to the in-kernel WireGuard handler.

I've built a proof-of-concept in Go here: https://github.com/stv0g/cunicu/tree/6965a8c42b8a2663b7fea7de2695a1b6c27cd29d/ebpf_poc

https://app.codecov.io/gh/stv0g/go-rosenpass/tree/master/config

netbird / cunicu have the requirement to manage single peers at runtime without restarting the full server and re-negotating all peers.

API proposal

func (s *Server) AddPeer(pcfg *PeerConfig) error {

}

func (s *Server) RemovePeer(pcfg *PeerConfig) error {

}

// BUG(rsc,mikio): On DragonFly BSD and OpenBSD, listening on the
// "tcp" and "udp" networks does not listen for both IPv4 and IPv6
// connections. This is due to the fact that IPv4 traffic will not be
// routed to an IPv6 socket - two separate sockets are required if
// both address families are to be supported.
// See inet6(4) for details.

Source: https://go.dev/src/net/ipsock.go

Workaround: Open two sockets for DragonFly and OpenBSD.

This issue does not seem to affect Windows.

By @koraa in #27 (comment)

Severerity: N/A – Is it possible to write allocation-free code in go? The implementation must not have network-dependent memory usage and it should not allocate in response to network messages.

By @koraa in #27 (comment)

https://github.com/stv0g/go-rosenpass/blob/d7e38ecaf9e7803f2824a03ac24ac34944a53af6/server.go#L251

Severity: N/A – How do you make sure all these locks never cause a dead-lock situation?

https://github.com/spf13/cobra/blob/main/site/content/completions/_index.md

By @koraa in #27 (comment)

Severerity: N/A – What is the situation with regards to buffer overflows, use-after-free errors in go?