search

icloud-photos-downloader / icloud_photos_downloader

A command-line tool to download photos from iCloud
MIT License
6.92k stars 556 forks source link

MFA/2SA code entry bug #925

Closed boredazfcuk closed 3 months ago

boredazfcuk commented 3 months ago

Overview

There seems to be a bug when entering a code which starts with a digit which is also one of the trusted SMS index numbers.

Steps to Reproduce

  1. Attempt re-authorisation on an account which has a trusted number associated 2: 2024-07-31 23:41:30 DEBUG Authenticating... 2024-07-31 23:41:31 INFO Two-factor authentication is required (2fa) 0: * ***12 1: ****34 Please enter two-factor authentication code or device index (0..1) to send SMS with a code:
  2. Enter code 000000
  3. Receive message: Please enter two-factor authentication code that you received over SMS:

Expected Behavior

I expect icloudpd to attempt to authenticate to icloud using 000000 as the six digit MFA code

Actual Behavior

icloudpd recognises 000000 as 0 and an SMS with an MFA code is received to the mobile device number in index position 0

Context

I was attempting to simulate an MFA failure, so I could check the exit code of icloudpd, and potentially use this to confirm a successful/unsuccessful MFA attempt with my remote authentication feature. I found that entering an MFA code of 000000 sent an SMS to my mobile device, instead of processing it as a code.

I have seen a similar bug has been fixed in v 1.21.0, which allows MFA codes to have leading zeros: https://github.com/icloud-photos-downloader/icloud_photos_downloader/pull/891. I am currently running v1.22.0 so this is a separate, but similar issue. It would seem that MFA codes with leading zeros (and presumably leading 1s, 2s, 3s etc) may be mistaken for trusted device index numbers. Presumably this section of codes needs to be amended to check the input string length. If the string is a single digit, it is to be used as an SMS number index value. If it is 6 digits, it is recognised as an MFA code and used appropriately.

AndreyNikiforov commented 3 months ago

Your research is valid. Index and MFA are using the same domain and index is first 99 and the rest is for MFA code, so int(MFA) > 99. That is not a great design in hindsight. Probably better to separate by type, e.g. index is alphas a..z and mfa just numbers.

I think severity of this is issue is low though. Most users will have one device (index==0) and a chance of MFA beings 000000 is slim (==I suspect it may be even prohibited special case).

AndreyNikiforov commented 3 months ago

fixed in 1.23.1