tabnews.com.br icon indicating copy to clipboard operation
tabnews.com.br copied to clipboard

A validação de email+senha deve tomar o mesmo tempo quando o usuário existe ou não

Open tcarreira opened this issue 3 years ago • 17 comments

De forma a deixar a rota /api/v1/sessions uma black box (ou seja, não ser possível saber se um usuário existe ou não através do tempo de resposta do servidor), precisamos fazer algumas alterações.

Sugestão de fluxo de validação de login, em pseudo-código

startTime = getMonotonicTime()
targetElapsed = random(2.5, 3.5) // uma duração de tempo aleatória entre 2.5s e 3.5s

userFromDB = getUserByUsername(requestedUser)
if (userFromDB) {
  isMatch = checkPasswordMatch(userFromDB, requestedPassword)
}

await waitForElapsed(startTime, targetElapsed) // espera até terem passado pelo menos entre 2.5s e 3.5s desde o startTime

if isMatch {
  return true
}
return false

Vantagens:

  • a rota é consistentemente lenta, previne timing attacks
  • o hash da senha só é calculado quando o usuário existe (fazer o hash é caro)
  • aquele waitForElapsed() pode ser implementado com um sleep, o que reduz o CPU usado (fica mais barato quando o usuário não existe)

tcarreira avatar Feb 13 '22 13:02 tcarreira

@coffee-is-power por que essa issue foi fechada?

filipedeschamps avatar Jul 12 '22 12:07 filipedeschamps

A feature ja foi implementada

coffeeispower avatar Jul 12 '22 12:07 coffeeispower

Onde exatamente?

filipedeschamps avatar Jul 12 '22 12:07 filipedeschamps

#171 E tambem essa issue ja esta parada a 2 meses, se o criador da issue quiser voltar e so pedir para abrir

coffeeispower avatar Jul 12 '22 12:07 coffeeispower

@coffee-is-power não entendi. Onde no código (ou até naquela issue), demonstram que "A validação de usuário+senha deve tomar o mesmo tempo quando o usuário existe ou não" foi implementado?

filipedeschamps avatar Jul 12 '22 12:07 filipedeschamps

Edit: Como o login agora utiliza o email, e não mais o username, o que eu argumentei abaixo não se aplica. E a implementação sugerida se justifica sim, na verdade, ainda mais do que antes.

Meus 10 centavos para esse assunto é que não existe justificativa para essa implementação enquanto houverem outras formas simples de obter os usuários válidos, como nesse endpoint:

https://www.tabnews.com.br/api/v1/users

E mesmo se eliminasse a rota acima, bastaria o atacante ir testando esse outro endpoint antes de tentar descobrir a senha:

https://www.tabnews.com.br/[username]

aprendendofelipe avatar Jul 14 '22 23:07 aprendendofelipe

@aprendendofelipe alguns pontos importantes nessa questão:

  1. A lista de usuários não faz diferença como vetor de ataque nessa situação, pois a nossa autenticação não é mais feita usando o username como antigamente, ela é feita apenas usando o email. Como você falou, o atacante pode fazer o scrape do username de outras formas, por isso foi importante alterar a autenticação para apenas email, uma vez que isso é um dado privado e não é exposto em nenhum lugar do TabNews. Isso foi uma sugestão do @tcarreira feita na mesma bateria de sugestões dessa issue daqui.
  2. Então esta implementação não tenta evitar a listagem dos usuários, mas tenta evitar que o atacante saiba da existência de um email na base, pois se o email não existe, a resposta vai ser muito mais rápida. Isso porque do jeito que foi implementado, o cálculo do hash da senha, que é uma etapa devagar, só acontece se o email foi encontrado.
  3. Então ele sabendo da existência ou não de um email, o atacante pode escolher se focar em atacar a senha. Por exemplo, o meu email público aqui no GitHub é [email protected] e o atacante pode hoje tentar descobrir se eu usei esse email ou não no TabNews. Se responder muito rápido na tentativa de criar a sessão, ele sabe que eu não usei e nem adianta tentar atacar a senha, mas se responder devagar, bingo.
  4. Isso também pode ser usado quando há vazamento de email+senha de outros lugares. Isso acontecia muito no Pagar.me, onde um e-commerce qualquer tinha um vazamento da base de usuários, o pessoal quebrava o hash e começava a testar essa combinação contra o nosso endpoint de autenticação para ver se por sorte algum usuário tinha usado o mesmo email e senha. Na época que eu trabalhava lá implementamos um rate limiting que identificava esse comportamento e continuava retornando uma resposta de erro, mesmo que o atacante tinha acertado a combinação. Enfim, na situação atual do TabNews, dá para identificar que pelo menos o mesmo email foi utilizado num caso de vazamento de lista de emails.
  5. De qualquer forma, o endpoint /user talvez deveria ser removido por enquanto até que a gente tenha um caso concreto que necessite dele para algum client, e principalmente quando ele tiver paginação. Então @aprendendofelipe sugiro não aplicar o PR #494 e fazer um outro que remova (desliste) esse endpoint. E quando o endpoint voltar, ele poderia ser listado de forma pública sem problemas, o GitHub faz isso também e no caso deles as pessoas já construíram coisas legais em cima 👍
  6. Mas no final das contas, precisamos implementar rate limiting, e isso já está listado como tarefa nessa Milestone 🤝

filipedeschamps avatar Jul 15 '22 02:07 filipedeschamps

Corretíssimo @filipedeschamps! Eu vi o getUserByUsername no pseudo código (que o @tcarreira deve ter escrito antes da alteração para email) e nem lembrei que não usamos o username no login 😅

Sobre retirar o endpoint users, você que manda! 👍

Mas pensa só um pouco antes, pois pode ser útil manter o acesso somente para você (desde que adicione a feature read:user:list ao seu usuário).

Da maneira que ficou no #494, ninguém mais terá acesso, já que nenhum usuário tem essa feature.

E também já está com os testes que vão impedir que a rota seja exposta novamente.

aprendendofelipe avatar Jul 15 '22 03:07 aprendendofelipe

Da maneira que ficou no https://github.com/filipedeschamps/tabnews.com.br/pull/494, ninguém mais terá acesso, já que nenhum usuário tem essa feature.

Puts, é verdade! Sensacional!!!! Acabei de editar o draft da Milestone 5 com esse PR 🤝 🤝 🤝

filipedeschamps avatar Jul 15 '22 04:07 filipedeschamps

O e-mail também é dito que existe na hora do cadastro: image Para resolver isso, logo após enviar o formulário de cadastro, redirecionar o usuário para a confirmação de e-mail independente se já existir um e-mail cadastrado, mas com a seguinte mensagem "Iremos enviar um e-mail de validação caso ainda não esteja cadastrado". Assim, no back-end, iremos enviar a confirmação apenas se o e-mail não existir no banco de dados.

ThallesP avatar Sep 19 '22 18:09 ThallesP

@ThallesP show! Uma dúvida que tenho sobre esse ponto de evitar a conferência do email é que muita gente fala para fazer isso na hora do cadastro, mas ainda fica um furo que é na hora da atualização por email, pois esse é outro vetor. Qual a melhor UX nesse caso?

Por exemplo:

filipedeschamps avatar Sep 19 '22 18:09 filipedeschamps

@filipedeschamps Pensei em duas opções:

  • Usar quase mesmo esquema que eu disse para o cadastro, e ao atualizar o e-mail, pedir a verificação para esse novo e-mail e também dar a opção do usuário fornecer outro e-mail dentro dessa nova verificação (caso ele tenha errado na hora de atualizar). (minha opção favorita)
  • Dizer que o e-mail existe, mas colocar um rate-limit para alterar (1 dia ou 3 dias?) raramente mudamos de e-mail então acredito que não teria problema.

Problema que eu vejo com a primeira opção é caso um usuário mal intencionado tente fazer spam atualizando para e-mails aleatórios, talvez precise de um rate-limit para prevenir isso.

ThallesP avatar Sep 19 '22 18:09 ThallesP

Tentando organizar as Issues, acredito que avisar que o e-mail já está cadastrado é uma funcionalidade muito importante. o github faz assim hoje.

Captura de tela em 2023-03-23 23-59-18

Sobre o tempo de login com usuário válido e não valido, já foi implementado?

rodrigoKulb avatar Mar 24 '23 03:03 rodrigoKulb

Tentando organizar as Issues, acredito que avisar que o e-mail já está cadastrado é uma funcionalidade muito importante. o github faz assim hoje.

Sobre o tempo de login com usuário válido e não valido, já foi implementado?

Ainda não foi implementado. E são duas medidas contraditórias, pois o login é com email, então se for mostrar que o email já existe, não é necessária a implementação dessa issue.

Mas veja que, por estas mensagens do @filipedeschamps, a intenção da issue é justamente esconder a informação se o email já estiver cadastrado:

2. Então esta implementação não tenta evitar a listagem dos usuários, mas tenta evitar que o atacante saiba da existência de um email na base...

3. Então ele sabendo da existência ou não de um email, o atacante pode escolher se focar em atacar a senha...

4. Isso também pode ser usado quando há vazamento de email+senha...

Mas tem que ver se é possível fechar todos os furos, pois, se não for possível, talvez seja melhor agir no sentido de impedir a descoberta da senha por força bruta. Já existe o rate-limit, mas como é por IP, não pode ser muito restrito, então deveria haver um limite por usuário.

aprendendofelipe avatar Mar 24 '23 04:03 aprendendofelipe

@aprendendofelipe obrigado pelas explicações:

  • Realmente a funcionalidade de informar que o e-mail já está em uso é essencial.

Desta forma, realmente não faz sentido quebrar a cabeça com o time de login para verificar se o e-mail existe. Então não vejo necessidade de manter essa Issue aberta, tendo que o título dela é para alterar o tempo quando o usuário existe ou não.

Estou nessa Issue porque é a mais antiga aberta do projeto, e vou fazer uma "limpeza" da mais antiga para frente.

👍️ - Finalizar essa Issue. 👎️ - Seguir com ela aberta.

rodrigoKulb avatar Mar 24 '23 11:03 rodrigoKulb