読者です 読者をやめる 読者になる 読者になる

XSSを発見した。またはJSダブルクオートのススメ

もう修正・対策済みなので喋ってしまいますが、自社のサービスにXSS脆弱性を見つけてしまったのです。

サーバサイドは node.js で、テンプレートエンジンは jade(pugjs)。問題のコードを簡略化して書くと以下のようになります。

head
  script
    var hoge = {
       a: '#{param}',
       b: 'fuga'
    }; 

恐ろしい事に、このparam変数にはURLのクエリストリングの値が入るのです。はい、もう嫌な予感しかしないですね〜

jadeは < > % " をエスケープしますが、シングルクオートはエスケープせず出力されてしまいます。

ということは、以下のようなパラメータを渡すと ' がそのまま出力されてしまうため window.alert が実行されてしまうんじゃないですかねー。

http://localhost:3000/login?param=',x:window.alert('xss'),y:'

生成されるHTMLを整形すると…

<head>
  <script>
    var hoge = {
       a: '',
       x: window.alert('xss'),
       y: '',
       b: 'fuga'
    }; 

かくして見事にalertが表示されるわけですが、今時こんなでっかい穴が2年も放置されてたなんてビックリですよ。これを利用して偽のログイン画面に誘導し、IDとパスワードを奪うといった攻撃が可能だったのでしょう。

こんなコードが入り込んだのは、コードレビュー体制が整ってない時代だったから。入力値を見たら攻撃できないか?とか真っ先に考える自分みたいなヤバい人のチェックが入らなかったんだろう。レビュー重要。

それとシングルクオートをエスケープしないテンプレートエンジンは結構あります。JS界隈はシングルクオート派が多いのですが、HTMLファイルにJSを書く時は、動的出力も考慮してダブルクオートがいいかもしれんですね。

parseIntを小数点切り捨てに使わない

JavaScript

TwitterのTL上でにわかに話題になっていた。parseIntは引数をstringにしてからint変換するので、小数点以下の切り捨てには使わん方が良いというお話。

> parseInt(0.000001)
0
> parseInt(0.0000001)
1

これは以下のように処理されているのと同じ

> (0.000001).toString();
'0.000001'
> parseInt('0.000001');
0
> (0.0000001).toString();
'1e-7'
> parseInt('1e-7');
1

ES2015対応のブラウザやnode v4系以降ならば、代わりに Math.trunc を使うと良いと思います。

元来 parsetInt は文字列を解析して整数を返す関数で、小数点以下を切り捨てる関数ではありません。

プログラミングの世界でparseという言葉は、主に文字列を解析して何かをする機能に対して使われる単語です。関数の正確な仕様を把握していなかったとしても(もちろん把握している方が良いのですが)、parseホゲホゲという関数に数値を渡すのは間違ってるのでは?と判断できる感覚を身に付けてしまうと良いと思います。