It is possible to verify a phone number for SMS verification without ever verifying it

[Bertg]

We have discovered a way that Rodauth allows SMS setup to happen with any phone number, without verification. The user story is something like this: (this is the issue we confirmed, it may be more wide spread).

Given I'm a user and I already have OTP codes set up
When I choose to start the setup of SMS based two factor
But I get interrupted before I can confirm with the code
And, later, I sign into my account again with OTP
Then the SMS setup is confirmed

The issue here is the sms_failures_column does two things.

• It keeps track of failures
• It keeps track of the fact that a code is in setup or not. See sms_needs_confirmation?.

In itself that is not an issue. However if we look at two_factor_authenticate we see a call to two_factor_remove_auth_failures. This method is implemented by all OTP methods, including the SMS implementation:

    def sms_remove_failures
      update_sms(sms_failures_column => 0, sms_code_column => nil)
    end

By setting the sms_failures_column to 0 the application has now, by accident, confirmed the phone number.

In hindsight there is also a code smell in the sms_confirm method.

    def sms_confirm
      sms_remove_failures
      super if defined?(super)
    end

Removing failures to confirm a phone number is one thing, but noting else happens. This code literally reads that confirming sms and removing failures is the same. And it really shouldn't be.

A quick solution would be to make the sms_remove_failures method check if sms_failures_column is not null. And write a separate method to write the sms_confirm to the database. I think it's best to keep these 2 codepaths separated.

The cleanest way would be to have a non ambiguous way of storing what rows in the sms_codes_table are for verification and which ones are for authentication.

[Bertg]

Additionally, we have found some other missing behaviour. This we did not confirm in a "normal" Rodauth setup. Ours is modified quite a bit, so I'm not sure if these issues are valid, for handled in a way that we broke in our setup:

When a user starts a SMS setup, and never completes it:

• it is impossible to try a setup with a new phone number
• it is impossible to try and setup with the same number at a later time when a new SMS would be required

According to us the setup mode, once initiated can not be canceled or restarted. It also doesn't time out.

I think a healthy solution should be able to:

• Allow to restart the setup with a new number
• Allow a resend of a token to the same number
• Set a timeout on the validity of a token sent for verification

[jeremyevans]

Thanks for the report. I agree this is a bug. The best way to fix this in a backwards compatible manner (no extra database columns) is to have sms_remove_failures exclude rows with nil failures, and for sms_confirm to set failures from nil to 0.

What are your thoughts on this approach:

diff --git a/lib/rodauth/features/sms_codes.rb b/lib/rodauth/features/sms_codes.rb
index 33a6c00..27ea526 100644
--- a/lib/rodauth/features/sms_codes.rb
+++ b/lib/rodauth/features/sms_codes.rb
@@ -362,16 +362,16 @@ module Rodauth
     def sms_setup(phone_number)
       # Cannot handle uniqueness violation here, as the phone number given may not match the
       # one in the table.
-      sms_ds.insert(sms_id_column=>session_value, sms_phone_column=>phone_number)
+      sms_ds.insert(sms_id_column=>session_value, sms_phone_column=>phone_number, sms_failures_column => nil)
       remove_instance_variable(:@sms) if instance_variable_defined?(:@sms)
     end

     def sms_remove_failures
-      update_sms(sms_failures_column => 0, sms_code_column => nil)
+      update_hash_ds(sms, sms_ds.exclude(sms_failures_column => nil), sms_failures_column => 0, sms_code_column => nil)
     end

     def sms_confirm
-      sms_remove_failures
+      update_hash_ds(sms, sms_ds.where(sms_failures_column => nil), sms_failures_column => 0, sms_code_column => nil)
       super if defined?(super)
     end

I'm open to potential improvements to the sms_codes feature, but they need to be backwards compatible (no new database column). Any improvements that are not backwards compatible must be implemented as a new feature that depends on sms_codes.

[Bertg]

I'm open to potential improvements to the sms_codes feature, but they need to be backwards compatible (no new database column). Any improvements that are not backwards compatible must be implemented as a new feature that depends on sms_codes.

As I see it, after this fix, the main issue that needs to happen is that the user can cancel out of the setup process. This would be very similar to the sms_disable endpoint I assume. Or could be a delete route on sms_confirm.

It would also be good for this to happen automatically after some specified time. This could be done similar to the deleting inactive sessions, where before any sms_setup route a call to a "delete all outdated sms setup codes" query (pseudo query: DELETE FROM sms_codes WHERE sms_failures_column IS NULL AND sms_issued_at_column > 10_MINUTES_AGO)

As we use only the API internally it's not something we will be able to do quickly. So I'll leave that exercise to the reader :)

[jeremyevans]

Agreed that it would be a useful feature to allow cancelation of the setup process in order to register a new number (due to a typo or other issue during initial setup). However, this must be done in a way that limits abuse, so that a number that hasn't been confirmed cannot get continually spammed with SMS messages. I think we can use the code_issued_at column to determine whether it is allowable to cancel unconfirmed sms setup. After some configurable period of time, if the SMS has not been confirmed, the user can cancel SMS setup via sms_disable (or maybe implicitly in the sms_setup route), and then proceed to sms_setup to try again with a new number.

[Bertg]

@jeremyevans As previously mentioned, we have a highly custom setup, so a PR for the above topic is not in our scope right now. However, I did want to share the "patch" we did to our system (this patch is credited to @butsjoh):

It has some application and rails specific hacks in it, but I think it can still be valuable.

r.on "auth/sms-setup" do
    rodauth.require_account
    rodauth.require_sms_not_setup

    r.post do
      requested_verification_phone_number = rodauth.send(:sms_normalize_phone, rodauth.param(rodauth.sms_phone_param))

      if !rodauth.sms_valid_phone?(requested_verification_phone_number)
        return {error: rodauth.sms_invalid_phone_message}.as_json
      end

      existing_verify_sms = rodauth.sms
      existing_phone_number = existing_verify_sms ? existing_verify_sms[rodauth.sms_phone_column] : nil

      minimum_resend_interval = 1.minute

      if !existing_phone_number || existing_phone_number != requested_verification_phone_number
        rodauth.send(:transaction) do
          rodauth.sms_disable if existing_verify_sms
          rodauth.send(:before_sms_setup)
          rodauth.sms_setup(requested_verification_phone_number)
          rodauth.sms_send_confirm_code
          rodauth.send(:after_sms_setup)
        end
      elsif existing_verify_sms[rodauth.sms_issued_at_column] < (Time.now.utc + minimum_resend_interval)
        rodauth.send(:transaction) do
          rodauth.sms_send_confirm_code
        end
      end

      {success: true}.as_json
    end
  end
end

[jeremyevans]

Thanks for sharing your implementation. I plan for a simpler approach. After a configurable deadline (default to 1 day to limit abuse), SMS confirm code automatically expires, and sms-setup can be used to resetup SMS authentication. The issue I see in your approach is you can spam SMS messages to an unauthenticated number every minute.

[Bertg]

@jeremyevans Yeah, that's absolutely true. In our business context that is less of an issue.

I just thing that 24 hours out of the gate is a very big punishment for a user that did a genuine typo when entering the data. Not only that, but its (for some users) then also 24h that they may not have a properly secured account.

We chose the 1.minute interval to prioritise well acting users that made a genuine mistake. If we would find abuse, I would introduce something like rack-attack for the endpoint specifically and protect for abuse that way.

But I understand the difficulty as the gem author to decide here what is more important... I don't envy that position.

[jeremyevans]

My view is that 24 hours is much sooner than never (previous behavior), so it's still a large improvement from the user's perspective. It's easy to configure a lower limit for people that want it.

[Bertg]

My view is that 24 hours is much sooner than never

Can't argue with that one