x/upgrade/types, all: use of time.Time.IsZero() only works for zero value created variables, but will fail if from using time.Unix(0, 0); perhaps switch this check

[odeke-em]

Summary of Bug

TL;DR: Let's change the calls from if time.Time.IsZero() to be if time.Time.UnixNano() <= 0 to ensure that values obtained from time.Unix(zeroSecValue, zeroNsValue) are caught too, otherwise the code isn't protected from such code usages and will incorrectly validate a plan that is incorrect. See https://play.golang.org/p/FV519_Nva0L
This code should not validate and should instead fail to execute this clearly invalid plan, but unfortunately it does

func TestInvalidSlip(t *testing.T) {
        p := types.Plan{
                Name:   "erroneous",
                Time:   time.Unix(0, 0), // This could result from unmarshalling zero values
                Height: 0,
        }
        var ctx sdk.Context
        ctx = ctx.WithBlockTime(time.Now())
        require.False(t, p.ShouldExecute(ctx))
} 

--- FAIL: TestInvalidSlip (0.00s)
    plan_test.go:177: 
        	Error Trace:	plan_test.go:177
        	Error:      	Should be false
        	Test:       	TestInvalidSlip
FAIL
exit status 1
FAIL	github.com/cosmos/cosmos-sdk/x/upgrade/types	0.293s

Description

The code in x/upgrade performs validation to ensure that either time or height are set as per

cosmos-sdk/x/upgrade/types/plan.go

Lines 43 to 51 in a51eac4

	if p.Time.IsZero() && p.Height == 0 {
	return sdkerrors.Wrap(sdkerrors.ErrInvalidRequest, "must set either time or height")
	}
	if !p.Time.IsZero() && p.Height != 0 {
	return sdkerrors.Wrap(sdkerrors.ErrInvalidRequest, "cannot set both time and height")
	}
	if !p.Time.IsZero() && p.UpgradedClientState != nil {
	return sdkerrors.Wrap(sdkerrors.ErrInvalidRequest, "IBC chain upgrades must only set height")
	}

In Go, the way that we get time.Time.IsZero() to return true is if we retrieved time from:

1. var t time.Time
2. t, _ := time.Parse(INVALID_FORMAT)
3. time.Unix(SECONDS_SINCE_0001-01-01-00:00:00-to-1970, NANOSECONDS_SINCE_00-00-00-0000-to-1970)

and the usage of 2) time.Parse is properly well handled

$ git grep -n 'time.Parse'
types/utils.go:80:      t, err := time.Parse(SortableTimeFormat, str)
types/utils_test.go:75:         timeFromRFC, err := time.Parse(time.RFC3339Nano, tc.RFC3339NanoStr)
types/utils_test.go:77:         timeFromSDKFormat, err := time.Parse(sdk.SortableTimeFormat, tc.SDKSortableTimeStr)
types/utils_test.go:99: tm, err := time.Parse("Jan 2, 2006 at 3:04pm (MST)", "Mar 3, 2020 at 7:54pm (UTC)")
x/evidence/types/evidence_test.go:14:   n, _ := time.Parse(time.RFC3339, "2006-01-02T15:04:05Z")
x/evidence/types/evidence_test.go:40:   n, _ := time.Parse(time.RFC3339, "2006-01-02T15:04:05Z")
x/ibc/light-clients/07-tendermint/client/cli/tx.go:84:                  trustingPeriod, err := time.ParseDuration(args[1])
x/ibc/light-clients/07-tendermint/client/cli/tx.go:89:                  ubdPeriod, err := time.ParseDuration(args[2])
x/ibc/light-clients/07-tendermint/client/cli/tx.go:94:                  maxClockDrift, err := time.ParseDuration(args[3])
x/upgrade/abci_test.go:286:     ti, err := time.Parse(time.RFC3339, "2020-01-01T00:00:00Z")
x/upgrade/client/cli/tx.go:179:         upgradeTime, err = time.Parse(TimeFormat, timeStr)
x/upgrade/client/rest/tx.go:81:                 t, err = time.Parse(time.RFC3339, req.UpgradeTime)
x/upgrade/types/plan_test.go:21:        t, err := time.Parse(time.RFC3339, s)

The other way that this could happen is by time.Unix

$ git grep -n 'time.Unix' | grep -v test
simapp/state.go:32:			genesisTimestamp = time.Unix(FlagGenesisTimeValue, 0)
types/simulation/rand_util.go:91:	return time.Unix(unixTime, 0)
x/evidence/types/params.go:11:var DoubleSignJailEndTime = time.Unix(253402300799, 0)
x/ibc/core/04-channel/keeper/packet.go:217:			"block timestamp >= packet timeout timestamp (%s >= %s)", ctx.BlockTime(), time.Unix(0, int64(packet.GetTimeoutTimestamp())),
x/ibc/light-clients/07-tendermint/types/misbehaviour.go:52:	return time.Unix(0, minTime)
x/simulation/simulate.go:81:		genesisTimestamp.UTC().Format(time.UnixDate), genesisTimestamp.Unix(),
x/slashing/keeper/hooks.go:20:			time.Unix(0, 0),
x/slashing/spec/05_hooks.md:22:      JailedUntil         : time.Unix(0, 0),
x/slashing/spec/07_tombstone.md:97:state, we set `DoubleSignJailEndTime` to `time.Unix(253402300800)`, the maximum
x/staking/types/commission.go:24:		UpdateTime:      time.Unix(0, 0).UTC(),
x/staking/types/validator.go:57:		UnbondingTime:     time.Unix(0, 0).UTC(),

if mistakenly a zero value for minTime or an epoch value was zero, when used with time.Unix, we should instead use

if t.UnixNano() <= 0

as per https://play.golang.org/p/FV519_Nva0L

and failing code

Version

Tip on master a51eac4

For Admin Use

• Not duplicate issue
• Appropriate labels applied
• Appropriate contributors tagged
• Contributor assigned/self-assigned

[robert-zaremba]

@odeke-em Are you handling this? Is this really a security issue in master? You created an "artificial test" which breaks.

[odeke-em]

Thanks for the question, Robert. It isn’t an artificial test, it is an exhibit of what could go wrong if someone sent bytes that marshal to 0 for time.Unix — my task is to do find things that could break, cause unexpected behaviour and raise them :-)

…

On Wed, Dec 2, 2020 at 9:52 AM Robert Zaremba ***@***.***> wrote: @odeke-em <https://github.com/odeke-em> Are you handling this? Is this really a security issue in master? You created an "artificial test" which breaks. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#8058 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ABFL3V2ZLQSJJVR3W2BX53DSSZ5FTANCNFSM4UJRSSJQ> .

[robert-zaremba]

Thanks. We had a planning and wanted to arrange the tasks. Are you going to handle this task? If not you can assign it to me.

[odeke-em]

Please go ahead and take it on, thank you!

…

On Wed, Dec 2, 2020 at 11:18 AM Robert Zaremba ***@***.***> wrote: Thanks. We had a planning and wanted to arrange the tasks. Are you going to handle this task? If not you can assign it to me. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#8058 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ABFL3V6CN6P22ASAPTPUCLTSS2HI7ANCNFSM4UJRSSJQ> .