Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

회원 비밀번호 암호화 방법 개선 #894

Closed
wants to merge 4 commits into from
Closed

회원 비밀번호 암호화 방법 개선 #894

wants to merge 4 commits into from

Conversation

kijin
Copy link
Contributor

@kijin kijin commented Aug 10, 2014

이슈 #889 에서 말씀드렸던 개선안입니다. 몇 년 묵은 소원이었는데 이제야 시간을 내서 작업을 해봤네요.

기존 회원DB 및 다른 모듈들과 100% 호환성을 유지하면서, 회원 로그인 비밀번호 저장시 PBKDF2, bcrypt 등 강력한 암호화(해싱) 함수 사용을 가능하도록 하는 것을 목표로 합니다.

변경한 내역은 크게 3부분으로 나누어 설명할 수 있겠습니다.

첫째, 비밀번호 암호화(해싱) 기능을 전담하는 클래스 추가

새로 추가한 파일은 classes/security/Password.class.php입니다. PBKDF2 알고리듬처럼 이미 공개되어 있는 정보를 가져다 쓴 몇몇 부분을 제외하면 모두 제가 직접 구현하였으며, 이 풀리퀘를 받아들이실 경우 저작권은 네이버 XE개발팀에 드리겠습니다.

Password 클래스는 아래와 같은 기능을 수행합니다.

  • 현재 시스템에서 사용 가능한 해싱 함수의 목록을 작성하고 (getSupportedAlgorithms() 메소드) 그 중 가장 강력한 보안을 제공하는 알고리듬을 추천합니다. (getBestAlgorithm() 메소드)
  • 주어진 비밀번호를 주어진 알고리듬으로 해싱합니다. (createHash() 메소드)
  • 이렇게 생성하여 DB에 저장해 두었던 해시를 사용자가 입력한 비밀번호와 비교합니다. (checkPassword() 메소드)
  • 지원하는 해싱 알고리듬 목록은 아래와 같습니다:
    • md5 (기존 XE 기본값)
    • md5+sha1+md5 (기존 XE에서 $useSha1을 지정할 경우 사용하던 옵션)
    • PBKDF2 (PKCS#5 v2 표준, sha256 기반) PHP 5.1.2~5.3.6에서 추천
    • bcrypt PHP 5.3.7 이상에서 추천
    • 그 밖의 알고리듬들은 11월 12일에 삭제했습니다.
  • 해시를 넘겨주면 해시의 길이, 특정 문자열의 존재여부 등을 통해 어떤 알고리듬으로 생성된 해시인지 자동으로 파악합니다. (guessAlgorithm() 메소드) 따라서 새 방식으로 해싱된 비밀번호는 물론, 기존의 어떤 방식으로 해싱된 비밀번호라도 정확하게 확인이 가능합니다.
  • 일부 해싱 함수는 무작위로 생성된 솔트를 필요로 합니다. openssl 모듈, mcrypt 모듈, /dev/urandom, mt_rand() 등 현재 시스템에서 사용 가능한 가장 강력한 엔트로피 출처를 파악하여 각 함수가 필요로 하는 규격의 솔트를 자동으로 생성합니다. (createSecureSalt() 메소드) 이 기능은 비번 암호화 외에도 무작위로 생성된 문자열이 필요한 곳에서는 얼마든지 활용이 가능할 것으로 보입니다.
  • 테스트 편의를 위해, XE와 별도로 사용하더라도 모든 기능이 정상적으로 동작하도록 만들었습니다. 그러나 XE 환경에서 사용시 아래에서 설명드릴 설정에 따라 자동으로 알고리듬을 선택하는 등의 헤택(?)이 있습니다.

둘째, member 모듈에서 비밀번호 처리시 위의 클래스와 연동합니다.

  • memberModel 클래스의 isValidPassword() 메소드를 고쳐서 Password 클래스와 연동하도록 합니다. 기존의 방식으로 암호화된 비번과 새로 추가된 알고리듬까지 이 함수 하나로 모두 지원합니다.
  • memberModel 클래스에 hashPassword() 메소드를 추가하여 Password 클래스와 연동하도록 합니다. 문서, 댓글, 게시판 등의 모듈을 보면 비번을 확인할 때는 memberModel 클래스의 isValidPassword() 메소드를 호출하는데, 비번을 해싱할 때는 모두 각자 md5()를 사용하고 있습니다. 지금까지 XE에서 비번 확인 메소드는 제공했지만 비번 해싱 메소드는 별도로 제공하지 않았기 때문입니다. 해싱 메소드를 제공함으로써 앞으로 XE 모듈을 개발하시는 분들의 편의를 도모합니다.
  • memberController 클래스에서 회원 가입 및 직접 추가, 회원 정보 및 비번 변경, 비번 리셋 등의 작업을 할 때는 언제나 memberModel 클래스의 hashPassword() 메소드를 호출하도록 하여 일관성을 높였습니다.

셋째, 관리 페이지에서 해싱 방법을 직접 지정할 수 있도록 하였습니다.

회원 설정에 비밀번호 암호화와 관련된 설정 항목 3가지를 추가하였습니다. (스크린샷 참조)

untitled

  • 기존에 운영중이던 사이트에 패치를 적용하면 기본값은 md5로 설정됩니다. 기존 버전과의 호환성을 보장하기 위해서입니다. 그러나 패치를 적용한 후에는 목록에서 원하는 알고리듬을 선택하여 변경할 수 있습니다. (PHP 5.3.6 이하에서는 bcrypt 옵션이 나타나지 않습니다.)
  • 암호화 소요시간은 bcrypt의 work factor를 의미합니다. 416 범위에서 조절할 수 있으며, 기본값은 8입니다. 권장하는 범위는 812 사이입니다. (원래 bcrypt는 4~31 범위를 지원하지만, 16이 넘어가면 서버에 무리가 많이 갑니다. 너무 높은 값을 실수로 선택하는 것을 방지하기 위해 화면상에는 16까지만 표시합니다.)
  • PBKDF2에도 소요시간 설정이 동일하게 적용됩니다. 같은 설정 하의 bcrypt와 비슷한 시간이 소요되도록 했습니다.
  • 암호화 방법 자동 변환 설정은 현재 설정된 알고리듬과 다른 알고리듬으로 해싱된 비밀번호가 발견될 경우 자동으로 변환할지의 여부를 결정합니다. 물론 해싱 함수는 복호화가 불가능하므로, 다음번에 로그인하거나 비밀번호를 변경할 때 변환이 이루어집니다.

기타

  • 처음 설치할 때는 서버에서 지원하는 가장 강력한 알고리듬을 자동으로 선택합니다. PHP 5.3.7 이상에서는 bcrypt, 5.3.6 이하에서는 PBKDF2이지만, PHP 모듈이 이상하게 설치되어 있는 경우 기존과 같이 md5로 선택될 수도 있습니다.
  • 문서, 댓글, 게시판, 방명록, 위키, textyle 등 다른 모듈에는 영향을 주지 않습니다. (로그인 비밀번호를 제외하면 대부분의 모듈들이 자체적으로 md5()를 사용하고, 비번을 확인할 때만 memberModelisValidPassword() 메소드를 호출하고 있습니다. isValidPassword() 메소드는 기존의 md5 해시와 100% 호환되기 때문에 괜찮습니다.) 호환성 테스트에 대한 자세한 내용은 비밀번호 암호화 방법 개선 #889 를 참조하시기 바랍니다.
  • 모든 클래스와 메소드에는 철저하게 주석을 넣고, XE 코딩 스타일에 맞추려고 노력했습니다.

무지막지하게 큰 패치와 스크롤 압박 쩌는 풀리퀘 내용을 읽어주셔서 감사합니다. 호환성 문제가 있다거나, 설정을 조금 다른 방식으로 하면 좋겠다거나, 그 밖에 지적하실 부분이 있으면 얼마든지 알려주세요. 바꿀 건 바꾸고, 고칠 건 고치더라도, XE의 비번저장 보안은 꼭 개선되었으면 좋겠습니다.

그래도 저처럼 코어 개발자님들과 일면식도 없는 사람이 이런 풀리퀘도 넣을 수 있고... 이래저래 자유 소프트웨어 만세입니다 ^__^

@misol
Copy link
Contributor

misol commented Aug 10, 2014

반영되길 기원합니다.

@ghost
Copy link

ghost commented Aug 10, 2014

기대합니다.

@kijin
Copy link
Contributor Author

kijin commented Aug 10, 2014

password 필드의 길이를 업데이트하지 않은 사이트들에 대한 대책이 필요합니다.

MySQL은 varchar(60) 필드에 60자 이상을 넣어도 조용히 잘라버리고, 그 경우도 감안하여 프로그래밍을 하였습니다. 그러나 MS SQL은 60자 이상을 넣으면 에러가 발생하고, 큐브리드 8.4 미만 버전에서는 필드 길이 변경이 불가능하다고 합니다.

이 경우 현재 DB의 필드 길이를 초과하는 알고리듬은 선택하지 못하도록 하거나, 해시값을 저장하기 전에 미리 자르거나, 필드 길이 변경이 가능한 DBMS라면 알고리듬 선택 시점에서 필드 길이를 자동으로 늘려주는 방법을 찾아야겠습니다.

@lansi951
Copy link
Contributor

이렇게 한꺼번에 하면 확인하기 더 힘들어질 거 같네요

@kijin
Copy link
Contributor Author

kijin commented Aug 10, 2014

@lansi951 네, 풀리퀘가 상당히 크긴 합니다 ㅡ.ㅡ;;

그러나 워낙 서로 밀접하게 서로 연동되는 기능들이라, 세 부분을 따로따로 풀리퀘 할 경우 각각 테스트하기가 사실상 불가능합니다. 양해를 부탁드립니다.

@bjrambo
Copy link
Contributor

bjrambo commented Aug 10, 2014

@lansi951 커밋량보다 어차피 코어에 추가되는 코드는 한번에 봐야할 부분이 많아지면 역시 양이 많이질수박에 없지요,

언제 풀리퀘가 받아질지 모르겟찌만, 코드량이 많으면 그마만큼의 XE에서 점검시간이 길어질뿐 이부분의 대해서는 문제는 없으리라 생각되요

@kijin
Copy link
Contributor Author

kijin commented Aug 11, 2014

필드 길이 문제에 대한 업데이트입니다.

MySQL과 MS SQL은 필드 길이를 쉽게 변경할 수 있습니다.

큐브리드는 버전에 따라 필드 길이 변경이 불가능할 수도 있지만, 큐브리드 사용시 이미 XE에서는 password 필드를 60자가 아닌 180자로 생성하고 있습니다. (UTF-8 지원 문제 때문에 모든 varchar 필드를 3배 길이로 생성함) 이번에 추가한 알고리듬 중 해시 길이가 가장 긴 것은 170자(sha512+salt)입니다. 따라서 큐브리드에서는 컬럼 길이를 늘려줄 필요가 없습니다 ^^

feature/resizecolumn 브랜치에서 현재 사용중인 필드 길이를 정확하게 파악하는 루틴을 개발중에 있습니다. 이게 완성된다면 MySQL과 MS SQL에서도 현재 사용중인 필드 길이에 따라 암호화 방식 변경을 제한하거나 해시를 잘라주는 기능을 구현할 수 있게 될 것으로 보입니다.

@haegyung
Copy link

제가 요즈음, 예전에 저장된 비번들과의 충돌이라는 이슈가 생각나서 생각한건데...
기존 암호화를 사용한 USER 1 이 있으면

  1. USER1 로그인
  2. 비번 변경 페이지로 이동(강제)
  3. 비번 변경 페이지에서 비번 변경
  4. 비번 변경한 것을 USER1 의 비번으로 대치.

시키면, 이전에 사용하던 유저들의 비번도 싹 바꿀수 있지 않을까요?

@misol
Copy link
Contributor

misol commented Aug 11, 2014

암호를 바꾸는건 이미 기간에 따라 안내하도록 구현이 되어 있습니다.
사용자의 암호 변경은 본글의 이슈에서 벗어나는 내용 같습니다.
해시는 로그인 시점에 다시 해줄 수 있습니다.(기존 코드도 그랬던걸로 기억)

2014년 8월 12일 화요일, Hae, Gyungnotifications@github.com님이 작성한 메시지:

제가 요즈음, 예전에 저장된 비번들과의 충돌이라는 이슈가 생각나서 생각한건데...
기존 암호화를 사용한 USER 1 이 있으면

  1. USER1 로그인
  2. 비번 변경 페이지로 이동(강제)
  3. 비번 변경 페이지에서 비번 변경
  4. 비번 변경한 것을 USER1 의 비번으로 대치.

시키면, 이전에 사용하던 유저들의 비번도 싹 바꿀수 있지 않을까요?


Reply to this email directly or view it on GitHub
#894 (comment).

@kijin
Copy link
Contributor Author

kijin commented Aug 24, 2014

@ajkj 님의 제안대로 openssl_random_pseudo_bytes() 함수시 윈도우 사용 여부와 PHP 버전을 체크하도록 하고, 엔트로피 소스가 부족하여 mt_rand() 함수에 의존해야 할 경우에도 반드시 섞어서(mixing) 사용하도록 하여 예측 가능성을 차단하였습니다.

@ajkj
Copy link
Contributor

ajkj commented Aug 24, 2014

php 5.4.0 change log에 openssl_random_pseudo_bytes 를 이용하여도 window crypto api를 이용한다는 뜻의 글이 올라왔습니다. 그래서 저는 openssl_random_pseudo_bytes 의 속도저하 문제가 php의 문제가 아닌 openssl의 문제라서 php측에서 다른 방법(windows 에서는 windows crypto api 이용)으로 해결했다는 추측을 한 것 입니다. php 5.3.4 이후에도 속도 저하 문제가 발생하는지는 직접 테스트를 해 보는 방법 밖에 없는 것 같습니다.
http://php.net/ChangeLog-5.php#5.4.0

그리고 분명히 $_GET, microtime을 넣는다고, 암호학적으로 안전해진다는 보장은 없다는게 맞는 말씀 입니다. 쉽게 예측이 가능하겠지요. 다만 제가 그것을 넣은 이유는, 서버 및 웹사이트 별로 다른 random이 발생되도록 하는 것입니다. mt_rand의 기본 seed가 timestamp 와 pid 및 기타 쉽게 추측이 가능한 것을 집어 넣는것으로 알고있는데, 이러한 기본 seed는, 같은 시간대의 여러대의 서버에서 동시에 같은 값이 나올 가능성이 높다고 생각하였습니다.

그래서 $_server를 넣으면, 각 서버 및 웹사이트(도메인) 별로 최소한 다른값이 나오지 않을까 하는 생각이 들었습니다. 추가적으로 $_post, $_get, $_cookie등을 넣음으로서 이용하는 사용자의 요구별로 다른 값이 나오도록 하는것이 목적입니다.

json_encode를 사용한 이유는 serialize보다 약간 더 빠르다는 말이 있어서 이용하였습니다.

@kijin
Copy link
Contributor Author

kijin commented Aug 24, 2014

@ajkj 아, 역시 우리를 실망시키지 않는 PHP 코어 개발팀입니다. 윈도우7에서 PHP 모든 버전을 다운받아 openssl_random_pseudo_bytes() 함수를 32바이트씩 32번 호출해 보았는데...

  • PHP 5.3.3 이하 : 평균 23초
  • PHP 5.3.4 ~ 5.3.6 : 평균 67초 (고쳤다면서 오히려 더 느려졌음 ㅠㅠ)
  • PHP 5.3.7 ~ 5.3.29 : 평균 1.282초 (약간 고쳐진듯? changelog에는 관련 내용이 없는데?)
  • PHP 5.4.0 이상 : 평균 0.015초 (드디어 제대로 고쳐짐!)

역시 그 함수는 PHP 5.4 이상에서만 써야겠군요. 자세히 알려 주셔서 감사합니다.

그럼 이제 mt_rand()의 예측 가능성 문제로 돌아가 보죠. 기본 seed에는 현재 시간과 pid가 들어가는 것이 맞습니다. 그러나 해당 PHP 요청이 들어온 시간이 아니라 웹서버 프로세스가 시작될 때의 시간 중에서도 노이즈에 가까운 소수점 이하 유효숫자 기준이고, 웹서버 시작 후에도 종종 프로세스가 respawn되며, 각 프로세스의 mt_rand() 함수 호출 횟수도 제각각이므로 두 서버에서 동시에 같은 값이 나올 가능성은 매우 희박합니다. 만약 같은 값이 나온다면 알고리듬 자체의 약점보다는 우연의 일치일 가능성이 훨씬 높습니다. 애초에 이 함수가 반환할 수 있는 정수의 범위는 21억(2^31)에 불과하니까요. 조금 전 업데이트처럼 서로 다른 두 알고리듬으로 생성한 난수를 xor하여 사용하고 그것을 다시 해쉬함수로 mixing한다면 (두 방법 모두 RFC 4086에 나와 있습니다) 그 결과를 보고 이전 값이나 다음 값을 추측하기는 매우 어려울 것입니다. 여기에 각종 초전역변수까지 인코딩하여 추가하는 것은 카고컬트의 느낌이 강한 것은 물론이고, 성능 면에서도 얻는 것보다는 잃는 것이 더 많을 것 같습니다.

어쨌든 솔트에 사용하는 난수 생성 알고리듬의 보안성까지 신경을 쓴다면 openssl 또는 mcrypt 모듈을 설치하는 것이 정답입니다. mt_rand()로는 아무리 발버둥쳐도 한계가 있지요. 다행히 대부분의 웹호스팅 계정에서 mcrypt 모듈을 이미 지원하고 있으므로, mt_rand()에 의존해야 하는 경우는 극히 드물 것으로 생각됩니다.

@ajkj
Copy link
Contributor

ajkj commented Aug 24, 2014

말씀하신것을 들으니 mt_rand라 하더라도 겹치는 부분이 거의 없을것 같긴합니다.
그래도 방지 차원에서 넣는게 좋지 않을까요? 수십만대의 서버가 돌아가다 보면 몇대 정도는 동시에 똑같은 값이 나올 수 있을 것 같습니다. 아무리 mt_rand가 허술하다 해도 한 서버의 환경이 다른 서버에서 재현되는것을 놔두는 것은 좋은 방법이 아닌것 같습니다. 또한 mcrypt를 설치하지 않은 서비스도 고려해야 하니까요.

성능면에서 문제가 예상 된다면, 전부 다 넣지 말고, $SERVER의 HTTP_HOST,REQUEST_URI, REMOTE_ADDR 그리고 COOKIE 변수 또는 session id 정도만 salt로서 넣는것을 건의해 봅니다.

@kijin
Copy link
Contributor Author

kijin commented Aug 24, 2014

@ajkj 말씀하신 변수들은 하나도 빠짐없이 공격자가 알 수 있거나 심지어 조작까지 가능한 변수들입니다. 그 밖에도 $_SERVER에 들어있는 내용들은 거의 대부분 쉽게 예측할 수 있습니다. 따라서 이런 것들을 넣어봤자 별 도움이 되지 않는다는 제 입장에는 변함이 없습니다. 차라리 그냥 microtime()을 하나 더 넣고 말지요. (실제로 mixing 루틴 내에 rand()를 하나 넣어 두었습니다. 비슷한 효과입니다.)

mcrypt가 설치되어 있지 않더라도 리눅스를 비롯한 유닉스 계열의 서버 환경이라면 자동으로 /dev/urandom을 사용하도록 되어 있습니다. 결국 취약점이 발생할 만한 환경은 OS의 난수생성 API와 연동되는 rand() 함수조차 믿을 수 없을 만큼 아주 오래된 버전(2000, XP SP2 이전)의 윈도우뿐이고, 그것도 공격자가 서버 내에서 직접 프로그램을 실행할 수 있는 경우에 한정됩니다.

@ajkj
Copy link
Contributor

ajkj commented Aug 24, 2014

예전에 사용하던 library가 기억나서 한번 참고 차원에서 링크를 올려봅니다.
https://github.com/phpseclib/phpseclib/blob/master/phpseclib/Crypt/Random.php

@kijin
Copy link
Contributor Author

kijin commented Aug 24, 2014

장기적으로는 pbkdf2-compat, password_compat, RandomLib 등의 비번 암호화 및 난수생성 전문 라이브러리를 XE에 추가하고 이것들을 호출하도록 하는 것이 바람직하겠습니다. 그러나 XE는 PHP 5.2.4 이상 지원을 표방하므로, 이들 중 호환되지 않는 것이 많아서 상당부분을 직접 구현해야 하는 것이 현실입니다.

난수생성기 관련 부분은 PHPRandom에서 주로 작업을 하고 있습니다. 이쪽 풀리퀘에도 그때그때 변경사항을 적용하고 있으나, 이것도 장기적으로는 별도 라이브러리의 형태로 가져다가 쓰는 것이 낫겠습니다. 결정은 코어 개발자님들께 맡깁니다.

@kijin
Copy link
Contributor Author

kijin commented Nov 11, 2014

석 달이 넘었는데 아직도 코어 커미터분들로부터 아무 반응이 없으니 섭섭합니다.

된다, 안 된다, 이런저런 부분을 고쳐 달라, 고친 부분이 너무 많다, 하위버전 호환성이 걱정된다, 다른 계획이 있다, 다음 버전에 적용을 고려해 보겠다 등등... 이슈나 풀리퀘를 작성한 사람의 정성을 생각해서라도 한 마디쯤 의견을 표시해 주시면 얼마나 좋을까요?

사용자들도 줄곧 요구해 온 기능이고, 하위버전 호환성을 최대한 배려했으며, 정부 권고사항 준수를 위해서도 시급한 문제입니다. 모듈이나 애드온으로 만들 수 있었다면 벌써 전에 만들어 배포했겠지만, 문제의 특성상 코어 수정이 불가피해 보입니다. 그런데 코어 커미터분들은 너무 바쁘신 건지, 아니면 원래 성격이 무뚝뚝하신 건지, 석 달째 조용히 무시하고 계십니다. 이 풀리퀘만 그런 것이 아닙니다. 6개월 된 풀리퀘도 있고 1년 넘은 이슈도 있습니다.

소스만 공개되어 있다고 오픈소스가 아닙니다. 참여를 권장하기만 한다고 커뮤니티가 아닙니다. 양쪽으로 소통과 의견수렴이 이루어져야 하는데... 이곳 깃허브의 이슈와 풀리퀘 목록을 보면 코어 커미터분들은 가끔 라벨을 붙였다 뗐다 하시기만 하고, 커미터가 아닌 분들만 마치 담임선생님 없는 조회시간처럼 오손도손 얘기를 나누고 있습니다. 그나마 절반은 기능과 전혀 무관한 코딩스타일 문제로 트집을 잡곤 합니다.

리더에게 닿지 않는 소통에 무슨 의미가 있겠으며, 소통하지 않는 리더가 무슨 리더십을 발휘할 수 있겠습니까? XE의 고질병이라고 할 수 있는 만성소통결핍증을 이제는 좀 고쳐봐야 하지 않겠습니까?

물론 XEHub를 직접 방문하거나 XECon과 같은 행사에 참석할 수 있다면 좋겠지만, 지방이나 해외에 있어서 오프라인에서 만나기 어려운 사람도 온라인상에서 쉽게 소통할 수 있는 프로젝트가 되면 더욱 좋겠습니다.

@bjrambo
Copy link
Contributor

bjrambo commented Nov 11, 2014

코드 스타일을 지적할 수박에 없는 이유는 아래 주소와 같습니다.

http://www.xpressengine.com/devlog/22820642

XE코드 컨베이션이 정해져 있으니.. 그것을 따르는 것이 맞겠지요..

@kijin
Copy link
Contributor Author

kijin commented Nov 12, 2014

@qw5414 네, 코딩 스타일의 필요성은 잘 알고 있습니다. 제가 지적하려고 한 것은 "그나마 달리는 댓글도 절반은 코딩 스타일 얘기"이므로 정작 PR과 관련된 기능에 대한 논의는 거의 일어나지 않고 있다는 점입니다.

다른 PR도 그렇고, 이 PR도 마찬가지입니다. 실제로 코드를 뜯어보신 분은 @ajkj 님밖에 없는 것 같습니다. XEHub에 상주하신다는 개발자분들은 모두 어디에 가셨습니까?

@ghost ghost self-assigned this Nov 12, 2014
@ghost
Copy link

ghost commented Nov 12, 2014

@kijin 오랫동안 의견을 드리지 않아 죄송합니다.
우선, 내부 의견으로는 1.8버전에 반영 할 계획을 가지고 있습니다(1.8릴리즈 일정은 조정 중입니다).

제가 보기엔 너무 많은 선택지를 제공한다고 보여집니다.
시스템 환경(PHP 버전 등)에 따라 제공할 수 있는 적절한 방법 하나를 제공하는 것이 나아보입니다.

XE의 동작 PHP 최소 버전을 5.3으로 변경하는 방안을 고려 중이나, 저희가 파악하고 있는 실사용 환경과 1.8 일정에 따라 다시 판단 할 예정입니다.

차기 버전 관리에 다시 주력 할 예정이니 부디 계속 함께해주시길 부탁드립니다. (__)

@kijin
Copy link
Contributor Author

kijin commented Nov 12, 2014

@bnu 기다리던 답변입니다. 감사합니다.

불필요한 선택이 많아 보인다는 말씀에 동의합니다. 기존의 기본값(md5), PBKDF2, bcrypt 이렇게 3가지만 지원하고, PHP 버전에 따라 bcrypt (5.3.7 이상) 또는 PBKDF2 (5.3.6 이하)를 자동으로 선택하면 어떨까요?

현재의 PHP 최소 버전이 5.2.4이므로 최소한 PBKDF2가 지원되지 않을 일은 없을 것이고, PBKDF2는 해시 길이를 설정할 수 있으므로 password 필드의 길이조정도 필요하지 않겠습니다. 선택의 여지를 줄이면 여러모로 편리해지네요.

@dorami
Copy link
Contributor

dorami commented Jan 20, 2015

var $useSha1 = false;을 var $useSha1 = true; 로 변경하여 사용하는 유저가 추후에 암호화 방식을 변경하면 문제가 발생하진 않나요?

@kijin
Copy link
Contributor Author

kijin commented Jan 21, 2015

@dorami 맨 위의 "지원하는 해싱 알고리듬 목록" 부분에서 설명드린 것처럼, $useSha1 변수를 true로 했을 경우 사용되는 알고리듬(md5+sha1+md5)도 아무 문제 없이 지원합니다.

@dorami
Copy link
Contributor

dorami commented Jan 21, 2015

그 의미는 md5+sha1+md5 암호화를 쓰는 유저가 상위 암호화 방법(sha256 기반과 bcrypt등)으로 변경해도 문제가 발생하지 않는가요?

@kijin
Copy link
Contributor Author

kijin commented Jan 21, 2015

@dorami 정확히 어떤 시나리오에서 어떤 문제가 발생할 거라고 생각하시는지요?

@ghost
Copy link

ghost commented Jan 21, 2015

@kijin
Copy link
Contributor Author

kijin commented Jan 21, 2015

네, Password.php에서 이미 처리해 주고 있습니다.

return md5($password) === $hash || md5(sha1(md5($password))) === $hash;

sha1 옵션이 켜져 있든 꺼져 있든 상관없이, 항상 2가지 모두를 체크합니다.

@dorami
Copy link
Contributor

dorami commented Jan 21, 2015

괜한 질문을 드렸군요. 죄송합니다.

@kijin
Copy link
Contributor Author

kijin commented Jan 22, 2015

@dorami 아니오, 괜찮습니다. 조금이라도 문제가 발생할 가능성이 있다면 다시 한번 꼼꼼히 살펴보아야지요. 특히 $useSha1처럼 널리 알려져 있지 않은 옵션을 건드릴 경우에는 더욱더 호환성에 신경을 써야 합니다.

곰곰히 생각해 보니 한 가지 문제가 발생할 가능성이 있는데, $useSha1 = true로 되어있는 상태에서 이 PR을 적용하고 보안이 낮은 기본값(MD5)으로 암호화하도록 설정할 경우, 새로 저장되는 비번은$useSha1 옵션에도 불구하고 그냥 md5로 암호화된다는 점입니다. md5+sha1+md5로 이미 암호화되어 있는 비번을 체크하는 기능은 구현했지만, 새로 저장할 때 md5+sha1+md5로 암호화하는 옵션은 선택 수를 최소화하자는 @bnu 님의 의견에 따라 없애버렸거든요.

기존의 XE에서도 md5와 md5+sha1+md5를 모두 체크하도록 되어 있으므로 두 가지를 혼용한다고 문제가 발생하지는 않지만, 만약 회원DB를 다른 프로그램과 연동하고 있다면 다른 프로그램 쪽에서 문제가 발생할 수 있습니다.

(물론 다른 프로그램과의 DB 연동을 보장할 수 없다는 점은 비번 암호화 알고리듬을 다른 어떤 것으로 바꾸어도 마찬가지이겠지만, 방금 말씀드린 시나리오는 아무런 설정 변경 없이 XE를 버전업하기만 해도 문제가 발생할 수 있으므로 좀더 꼼꼼히 살펴볼 필요가 있겠습니다.)

위와 같은 문제의 소지를 없애려면 $useSha1 옵션을 되살리고, 이것이 true로 되어있는 경우 새로 저장되는 비번도 md5+sha1+md5를 사용하도록 하면 됩니다. 금방 수정이 가능할 것 같은데, 어떻게 생각하시는지요?

@lansi951
Copy link
Contributor

@kijin 전 없는 편이 좋겠네요. 알아서 체크가 된다면 별 문제가 없고
이 옵션을 잘 아는 사람이 없을 뿐더러 코어 업데이트 때도 불편해서 잘 사용되지 않는 것 같습니다.
그리고 보안을 강화하자는 이유로 새로운 암호화 알고리즘을 사용하는 건데
최대한 그걸로 유도하는게 좋죠

@dorami
Copy link
Contributor

dorami commented Jan 27, 2015

@kijin 다른 프로그램과 연동하는 케이스도 보기 힘든 경우이긴 합니다.
더 높은 암호화를 제공하는데, md5+sha1+md5를 유지할 필요는 없는거 같습니다.

@misol
Copy link
Contributor

misol commented Jan 27, 2015

@dorami 기존에 존재하던 옵션은 호환성 때문에라도 계속 가져가야 할거에요. XE에 zeroboard4 시절 mysql password 도 코드 찾아보면 흔적이 있는걸 보면요..

@kijin
Copy link
Contributor Author

kijin commented Jan 27, 2015

@misol 좀 생각해 보니 $useSha1 값을 그대로 반영하는 것도 문제가 될 수 있겠습니다.

이 PR을 적용한 후에는 게시판, 방명록 등 대부분의 모듈에서 비회원 비번을 저장하는 부분, 그 밖에 비번을 처리하는 모든 루틴들이 새 Password 클래스를 사용하도록 점차 마이그레이션하게 되겠죠?

그러나 기존의 XE에서 $useSha1 옵션은 member 모듈에서만 효과가 있습니다. 게시판, 방명록 등의 모듈들은 $useSha1 옵션을 무시하고 직접 md5를 호출하도록 하드코딩이 되어 있습니다.

따라서 $useSha1 옵션이 참이더라도 일괄적으로 md5+sha1+md5로 해싱을 했다가는 오히려 다른 모듈들과의 호환성에 문제가 생길 수 있습니다. 기존 방식에 일관성이 없었기 때문에, 일관성있는 알고리듬으로 대체해서는 안되는 거죠. (호출하는 모듈에 따라 차별대우하기도 골치아프고...)

어느 쪽을 선택하더라도 호환성을 손해볼 수밖에 없다면, $useSha1 옵션을 참으로 해두고 다른 프로그램과 연동까지 하는... 그야말로 아주 드문 케이스를 포기하는 편이 그나마 낫지 않을까요? 게다가 암호화 방식에 일관성이 없는 기존 XE의 특성상, 이런 케이스라면 연동하는 프로그램도 md5와 md5+sha1+md5를 모두 처리할 수 있도록 만들어져 있지 않을까 싶네요.

@misol
Copy link
Contributor

misol commented Jan 27, 2015


1번 해싱으로 일치하지 않으면 2번 해싱,
2번 해싱으로도 일치하지 않으면 3번 해싱,
3번 해싱으로도 일치하지 않으면 4번 해싱,
4번 해싱으로도 일치하지 않으면 5번 해싱,
...
n-1번 해싱으로도 일치하지 않으면 마지막 n번째 해싱 방법.
n번째 해싱 방법으로도 일치하지 않으면 일치하지 않는 비밀번호로 판정.

로그인에 성공하면, 1번 방식이 아니었던 경우 관리자가 설정한 방법으로 해싱 후 DB에 저장.


로그인 시에는 이렇게 하는게 어떤가요?
현재도 다른 방법일 경우 md5로 재 해싱하게 되어있어서 큰 무리 없지 않았나 싶습니다.??!?

2015년 1월 27일 화요일, Kijin Sungnotifications@github.com님이 작성한 메시지:

@misol https://github.com/misol 좀 생각해 보니 $useSha1 값을 그대로 반영하는 것도 문제가 될
수 있겠습니다.

이 PR을 적용한 후에는 게시판, 방명록 등 대부분의 모듈에서 비회원 비번을 저장하는 부분, 그 밖에 비번을 처리하는 모든 루틴들이
새 Password 클래스를 사용하도록 점차 마이그레이션하게 되겠죠?

그러나 기존의 XE에서 $useSha1 옵션은 member 모듈에서만 효과가 있습니다. 게시판, 방명록 등의 모듈들은 $useSha1
옵션을 무시하고 직접 md5를 호출하도록 하드코딩이 되어 있습니다.

따라서 $useSha1 옵션이 참이더라도 일괄적으로 md5+sha1+md5로 해싱을 했다가는 오히려 다른 모듈들과의 호환성에 문제가
생길 수 있습니다. 기존 방식에 일관성이 없었기 때문에, 일관성있는 알고리듬으로 대체해서는 안되는 거죠.

어느 쪽을 선택하더라도 호환성을 손해볼 수밖에 없다면, $useSha1 옵션을 참으로 해두고 다른 프로그램과 연동까지 하는...
그야말로 아주 드문 케이스를 포기하는 편이 그나마 낫지 않을까요? 게다가 암호화 방식에 일관성이 없는 기존 XE의 특성상, 이런
케이스라면 연동하는 프로그램도 md5와 md5+sha1+md5를 모두 처리할 수 있도록 만들어져 있지 않을까 싶네요.


Reply to this email directly or view it on GitHub
#894 (comment).

@kijin
Copy link
Contributor Author

kijin commented Jan 27, 2015

@misol 네, 해싱된 비번을 체크할 때는 이미 말씀하신 것과 같은 방법을 사용하고 있습니다. (단, 모든 알고리듬을 일일이 비교하지 않고 해시의 길이와 포함된 문자열 등을 통해 알고리듬을 추측한 후, 일치할 가능성이 있는 알고리듬만 시도합니다.)

@misol
Copy link
Contributor

misol commented Jan 27, 2015

그 이상은 필요 없다고 저도 생각합니다 @.@
sha1 = true 조건에 해당하는 해싱도 그 안에서 확인하면 되지요. 호환성이 어긋날 이유가 없어 보입니다.


  1. sha1 인 경우 로그인
  2. 로그인시 해싱 방법 변경됨
  3. 문제 없이 마이그레이션 완료.

2015년 1월 27일 화요일, Kijin Sungnotifications@github.com님이 작성한 메시지:

@misol https://github.com/misol 네, 해싱된 비번을 체크할 때는 이미 말씀하신 것과 같은 방법을
사용하고 있습니다.


Reply to this email directly or view it on GitHub
#894 (comment).

@kijin
Copy link
Contributor Author

kijin commented Jan 27, 2015

@misol 동의합니다. XE 내부에서는 전혀 문제될 것이 없지요. 위에서 논의한 호환성 문제는 다른 프로그램과 회원DB를 연동하는 경우에 문제가 생길 이론적인 가능성이 있을 뿐이고요.

@bjrambo
Copy link
Contributor

bjrambo commented Aug 10, 2015

한가지 궁금한게.. pbkdf2 방식을 bcrypt 방식으로 변경시에도 로그인이 되던 현상이 있던데, 이 방식이.. 암호화 방법이 바꿔었을때 전체적인 md5~bcrypt 전체적인 비밀번호를 다 해싱하여 매치하는지 검사를 하는 방식인건가요?

이게 궁금한 이유는.. 보통 설정을 변경하면, 기존 로그인자는
bcrypt 방식이 아닌, pbkdf2 의 방식의 비밀번호로 DB가 남겨졌을텐데, 로그인전까지는 그걸 모르고 사실상 암호화 방식을 매칭안한다고 다른 방식 찾아보다가 혹시나 하는 마음에 보안에도 문제 생기지 않을까 하는 우려때문에 질문드려보는거라서요..
(제가 솔찍히 보안쪽관련 몰라서.. 기존에 코드 꼼꼼히 다읽어봐도 모르겠더군요..그래서 한마디도 못하고 찍..)

@kijin
Copy link
Contributor Author

kijin commented Aug 10, 2015

@qw5414

해싱 알고리듬에 따라 결과의 포맷이 전혀 다릅니다. md5는 32글자의 16진수로 이루어져 있고, pbkdf2는 sha256:으로 시작하고, bcrypt는 $2y$로 시작합니다.

현재 설정과 무관하게 DB에 저장되어 있는 해시의 포맷을 기준으로 어떤 알고리듬으로 해싱되었는지 판단하고, 같은 알고리듬으로 해싱하여 비교합니다. 만약 해시가 sha256:으로 시작한다면 당연히 pbkdf2이므로 md5나 bcrypt는 시도하지도 않습니다.

자동 업그레이드 옵션을 선택한 경우에는 이렇게 판단한 알고리듬이 현재 설정과 동일한지 확인하고, 만약 다른 경우 현재 설정으로 다시 암호화하여 저장하는 과정이 추가됩니다.

마이그레이션 편의를 위해 기존 버전이 지원하던 MySQL PASSWORD() 함수도 여전히 지원하는데, 이것도 해시 포맷이 독특하기 때문에 딱 보면 알 수 있습니다. (버전에 따라 16진수 16글자 또는 *로 시작하는 41글자입니다.)

@bjrambo
Copy link
Contributor

bjrambo commented Aug 10, 2015

@kijin 아하 ! 그부분이 다른점이군요. 사실 저는 DB도 확인해봤지만 이 부분의 속성을 다 모르고 있다보니.. 혹시 bcrypt으로 저장햇던 비밀번호가 혹시, md5이랑 동일하게 되어서 로그인되는 현상이 생기거나 그런게 있을까 생각되었거든요..ㅎ 그게 아니였군요! 친절한 답변 감사합니다..

This pull request was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants