From 045765111a1e1b09c8e50559785e41f288f93dea Mon Sep 17 00:00:00 2001 From: Oliver Gugger Date: Thu, 12 Aug 2021 16:07:18 +0200 Subject: [PATCH] multi: use safe copy for macaroons Fixes #4383 by adding a new SafeCopyMacaroon function that correctly clones all caveats and prevents modifications on the copy from affecting the original. --- cmd/lncli/main.go | 5 +++- lnd.go | 5 +++- lntest/node.go | 5 +++- macaroons/auth.go | 15 ++++++++--- macaroons/constraints.go | 9 ++++++- macaroons/service.go | 18 +++++++++++++ macaroons/service_test.go | 55 +++++++++++++++++++++++++++++++++++++++ 7 files changed, 105 insertions(+), 7 deletions(-) diff --git a/cmd/lncli/main.go b/cmd/lncli/main.go index 0f2232766..83b9d1df5 100644 --- a/cmd/lncli/main.go +++ b/cmd/lncli/main.go @@ -167,7 +167,10 @@ func getClientConn(ctx *cli.Context, skipMacaroons bool) *grpc.ClientConn { } // Now we append the macaroon credentials to the dial options. - cred := macaroons.NewMacaroonCredential(constrainedMac) + cred, err := macaroons.NewMacaroonCredential(constrainedMac) + if err != nil { + fatal(fmt.Errorf("error cloning mac: %v", err)) + } opts = append(opts, grpc.WithPerRPCCredentials(cred)) } diff --git a/lnd.go b/lnd.go index a122e0002..81cd01e66 100644 --- a/lnd.go +++ b/lnd.go @@ -111,7 +111,10 @@ func AdminAuthOptions(cfg *Config, skipMacaroons bool) ([]grpc.DialOption, error } // Now we append the macaroon credentials to the dial options. - cred := macaroons.NewMacaroonCredential(mac) + cred, err := macaroons.NewMacaroonCredential(mac) + if err != nil { + return nil, fmt.Errorf("error cloning mac: %v", err) + } opts = append(opts, grpc.WithPerRPCCredentials(cred)) } diff --git a/lntest/node.go b/lntest/node.go index 71d2044e2..a38dab9f6 100644 --- a/lntest/node.go +++ b/lntest/node.go @@ -1195,7 +1195,10 @@ func (hn *HarnessNode) ConnectRPCWithMacaroon(mac *macaroon.Macaroon) ( if mac == nil { return grpc.DialContext(ctx, hn.Cfg.RPCAddr(), opts...) } - macCred := macaroons.NewMacaroonCredential(mac) + macCred, err := macaroons.NewMacaroonCredential(mac) + if err != nil { + return nil, fmt.Errorf("error cloning mac: %v", err) + } opts = append(opts, grpc.WithPerRPCCredentials(macCred)) return grpc.DialContext(ctx, hn.Cfg.RPCAddr(), opts...) diff --git a/macaroons/auth.go b/macaroons/auth.go index b8b5a8e8e..72a130d43 100644 --- a/macaroons/auth.go +++ b/macaroons/auth.go @@ -37,8 +37,17 @@ func (m MacaroonCredential) GetRequestMetadata(ctx context.Context, // NewMacaroonCredential returns a copy of the passed macaroon wrapped in a // MacaroonCredential struct which implements PerRPCCredentials. -func NewMacaroonCredential(m *macaroon.Macaroon) MacaroonCredential { +func NewMacaroonCredential(m *macaroon.Macaroon) (MacaroonCredential, error) { ms := MacaroonCredential{} - ms.Macaroon = m.Clone() - return ms + + // The macaroon library's Clone() method has a subtle bug that doesn't + // correctly clone all caveats. We need to use our own, safe clone + // function instead. + var err error + ms.Macaroon, err = SafeCopyMacaroon(m) + if err != nil { + return ms, err + } + + return ms, nil } diff --git a/macaroons/constraints.go b/macaroons/constraints.go index 58b369b2d..2cb36cacc 100644 --- a/macaroons/constraints.go +++ b/macaroons/constraints.go @@ -50,7 +50,14 @@ type Checker func() (string, checkers.Func) func AddConstraints(mac *macaroon.Macaroon, cs ...Constraint) (*macaroon.Macaroon, error) { - newMac := mac.Clone() + // The macaroon library's Clone() method has a subtle bug that doesn't + // correctly clone all caveats. We need to use our own, safe clone + // function instead. + newMac, err := SafeCopyMacaroon(mac) + if err != nil { + return nil, err + } + for _, constraint := range cs { if err := constraint(newMac); err != nil { return nil, err diff --git a/macaroons/service.go b/macaroons/service.go index b908fe521..3daccf0e2 100644 --- a/macaroons/service.go +++ b/macaroons/service.go @@ -275,3 +275,21 @@ func RawMacaroonFromContext(ctx context.Context) (string, error) { return md["macaroon"][0], nil } + +// SafeCopyMacaroon creates a copy of a macaroon that is safe to be used and +// modified. This is necessary because the macaroon library's own Clone() method +// is unsafe for certain edge cases, resulting in both the cloned and the +// original macaroons to be modified. +func SafeCopyMacaroon(mac *macaroon.Macaroon) (*macaroon.Macaroon, error) { + macBytes, err := mac.MarshalBinary() + if err != nil { + return nil, err + } + + newMac := &macaroon.Macaroon{} + if err := newMac.UnmarshalBinary(macBytes); err != nil { + return nil, err + } + + return newMac, nil +} diff --git a/macaroons/service_test.go b/macaroons/service_test.go index 00b8c9bf6..bf3cb0fde 100644 --- a/macaroons/service_test.go +++ b/macaroons/service_test.go @@ -253,3 +253,58 @@ func TestDeleteMacaroonID(t *testing.T) { ids, _ := service.ListMacaroonIDs(ctxb) require.Equal(t, expectedIDs[1:], ids, "root key IDs mismatch") } + +// TestCloneMacaroons tests that macaroons can be cloned correctly and that +// modifications to the copy don't affect the original. +func TestCloneMacaroons(t *testing.T) { + // Get a configured version of the constraint function. + constraintFunc := macaroons.TimeoutConstraint(3) + + // Now we need a dummy macaroon that we can apply the constraint + // function to. + testMacaroon := createDummyMacaroon(t) + err := constraintFunc(testMacaroon) + require.NoError(t, err) + + // Check that the caveat has an empty location. + require.Equal( + t, "", testMacaroon.Caveats()[0].Location, + "expected caveat location to be empty, found: %s", + testMacaroon.Caveats()[0].Location, + ) + + // Make a copy of the macaroon. + newMacCred, err := macaroons.NewMacaroonCredential(testMacaroon) + require.NoError(t, err) + + newMac := newMacCred.Macaroon + require.Equal( + t, "", newMac.Caveats()[0].Location, + "expected new caveat location to be empty, found: %s", + newMac.Caveats()[0].Location, + ) + + // They should be deep equal as well. + testMacaroonBytes, err := testMacaroon.MarshalBinary() + require.NoError(t, err) + newMacBytes, err := newMac.MarshalBinary() + require.NoError(t, err) + require.Equal(t, testMacaroonBytes, newMacBytes) + + // Modify the caveat location on the old macaroon. + testMacaroon.Caveats()[0].Location = "mars" + + // The old macaroon's caveat location should be changed. + require.Equal( + t, "mars", testMacaroon.Caveats()[0].Location, + "expected caveat location to be empty, found: %s", + testMacaroon.Caveats()[0].Location, + ) + + // The new macaroon's caveat location should stay untouched. + require.Equal( + t, "", newMac.Caveats()[0].Location, + "expected new caveat location to be empty, found: %s", + newMac.Caveats()[0].Location, + ) +}