Authentication bypass
Description
When adding an AllOfAuthenticator authenticator to an account, the only check on the provided data is that it unmarshals into an array of InitializationData.
func (aoa AllOfAuthenticator) OnAuthenticatorAdded(ctx sdk.Context, account sdk.AccAddress, data []byte) error {
var initDatas []InitializationData
if err := json.Unmarshal(data, &initDatas); err != nil {
return err
}
return nil
}This array could be empty or contain authenticator types that are not registered. When the Initialize is called, this could result in SubAuthenticators being empty as unregistered types will be ignored.
func (aoa AllOfAuthenticator) Initialize(data []byte) (iface.Authenticator, error) {
var initDatas []InitializationData
if err := json.Unmarshal(data, &initDatas); err != nil {
return nil, err
}
for _, initData := range initDatas {
for _, authenticatorCode := range aoa.am.GetRegisteredAuthenticators() {
if authenticatorCode.Type() == initData.AuthenticatorType {
instance, err := authenticatorCode.Initialize(initData.Data)
if err != nil {
return nil, err
}
aoa.SubAuthenticators = append(aoa.SubAuthenticators, instance)
}
}
}
return aoa, nil
}Impact
If the array of SubAuthenticators is empty then iface.Authenticated() will be returned, always allowing the account to be authenticated.
func (aoa AllOfAuthenticator) Authenticate(ctx sdk.Context, account sdk.AccAddress, msg sdk.Msg, authenticationData iface.AuthenticatorData) iface.AuthenticationResult {
allOfData, ok := authenticationData.(AllOfAuthenticatorData)
if !ok {
return iface.Rejected("invalid authentication data for AllOfAuthenticator", nil)
}
for idx, auth := range aoa.SubAuthenticators {
result := auth.Authenticate(ctx, account, msg, allOfData.Data[idx])
if !result.IsAuthenticated() {
return result
}
}
return iface.Authenticated()
}Recommendations
When adding a new AllOfAuthenticator to an account, ensure that the InitializationData array is not empty and that all types are registered.
Remediation
This issue has been acknowledged by Osmosis Labs, and a fix was implemented in commit 7d177171↗. It is now ensured that the AllOfAuthenticator and AnyOfAuthenticator have registered SubAuthenticators.