search

ProteGO-Safe / backend

GNU General Public License v3.0
71 stars 29 forks source link

Dalsze limitowanie wysyłania SMSów (spam protection) #53

Closed jakublipinski closed 4 years ago

jakublipinski commented 4 years ago

Zgodnie ze zmianam w tym PR: https://github.com/ProteGO-app/backend/pull/52

rtpm commented 4 years ago

Super, każde dodatkowe zabezpieczenie jest OK.

Popraw mnie proszę, jeśli blokuje to możliwość uruchomienia w jakiejś pętli jednego po drugim utworzenia procesu rejestracji na losowe numery (lub osób, które atakujący nie lubi..) i nie omijamy póki co rezultatu jakim jest otrzymanie przez dużą ilość osób SMS z kodem potwierdzającym.

Na pewno ma to implikacje zarówno dla tych, co już apki używają, jak i tych, co apki nie używają, ale na pewno w obu przypadkach niepotrzebny chaos (zapchana infolinia) i koszty (SMSAPI).

Myślę, że trzeba ograniczyć per minuta/godzina dla tego samego adresu IP samo zgłoszenie numeru telefonu do rejestracji celem ominięcia niepotrzebnie większej ilości rejestracji numerów. Obecne działania mocno operują lub podejmują akcje na podstawie już drugiego etapu (potwierdzenie samego numeru kodem). Nawet jeśli się znajdzie duża ilość użytkowników z tego samego IP (mało prawdopodobne), możemy w apce dać komunikat, by spróbować za np. 5/10 minut.

Mam świadomość, że uwzględniacie użycie unikalnego identyfikatora (API Androida/IOS), ale tak jak pisaliście to nie daje gwarancji ...

Największe bezpieczeństwo dałoby zapewne (tak jak pisałem) jakieś recaptcha jako potwierdzenie procesu rejestracji numeru.

bartekn commented 4 years ago

Miałem otwierać nowe issue ale dołączę się tutaj. Wydaje mi się że proces można by uprościć wyodrębniając rate-limiting na całe API (bo wyobrażam sobie że np. send_encounters też powinno być limitowane), dodatkowo każdy endpoint może mieć odrębny limit. Nie wiem czy GC ma taki produkt ale jeśli nie można postawić proxy server który będzie za to odpowiedzialny.

Po tej zmianie, w register można tak na prawdę trzymać jeden rekord na każdy numer z kodem i info kiedy został wygenerowany po raz ostatni i to powinno wystarczyć do implementacji wszystkich wymagań. Jako bonus nie będzie trzeba czyścić starych rekordów.

jakublipinski commented 4 years ago

@rtpm to wszystko co piszesz jest już w kodzie. Limitujemy per numer i per IP. Dobrze byłoby sprawdzić czy nie ma tam jakiejś luki. Docelowo recaptcha - zgadzam się ale w 1.0 tego nie zrobimy.

@bartekn nie do końca widzę potrzebę limitowania czegoś innego niż rejestracja. Rejestracja jest kluczowa ze względu na bezpieczeństwo, nadużycia (spamujemy ludzi kodami) i potencjalne koszty (wysyłamy miliony SMSów)

bartekn commented 4 years ago

nie do końca widzę potrzebę limitowania czegoś innego niż rejestracja

Inne endpointy to też koszty: API calls quota, storage. Pewnie mniejsze niż koszty SMS, ale np. streaming inserts to już $0.01 per 200 MB. To bardzo szybko poleci. Jakiś rate limiting oparty na memcache albo redis wyjdzie kosztowo o wiele taniej. Generalnie jeśli się tego nie zabezpieczy od razu to na pewno ktoś to wykorzysta jak appka będzie w produkcji.

jakublipinski commented 4 years ago

Issue zastąpione przez: https://github.com/ProteGO-app/backend/issues/65