自分がコーディングしたプログラムを自らコードレビュー（VB.NET）

レビュー対象のサンプルコード

以下のコードは、SQL Serverへ接続し、SQLを実行した結果をアプリケーション上へ取得する処理を記述しています。ユーザーIDとパスワードを条件として得られた結果を元に、認証してよいユーザーかどうかを処理するためのメソッドになります。

Imports System.Data.SqlClient

Public Class clsSqlServerRespoder

    Public Function getAutenticate(ByRef systemErrorFlag As Boolean, ByRef userID As String, ByRef password As String, ByRef isAuthenticated As Boolean) As Boolean
        Dim cn As New SqlClient.SqlConnection

        Try
            '環境変数から必要なデータを取得してくる
            Dim devDataSource As String = System.Environment.GetEnvironmentVariable("DEV_DATA_SOURCE")
            Dim devInitialCatalog As String = System.Environment.GetEnvironmentVariable("DEV_INITIAL_CATALOG")
            Dim devUserID As String = System.Environment.GetEnvironmentVariable("DEV_USER")
            Dim devPassword As String = System.Environment.GetEnvironmentVariable("DEV_PASSWORD")
            Dim devTimeout As String = System.Environment.GetEnvironmentVariable("DEV_TIMEOUT")

            'SQL Serverへの接続情報を作る
            Dim connectionString As String = ""
            connectionString &= String.Format("Data Source = {0};", devDataSource)
            connectionString &= String.Format("Initial Catalog = {0};", devInitialCatalog)
            connectionString &= String.Format("User ID = {0};", devUserID)
            connectionString &= String.Format("Password = {0};", devPassword)
            connectionString &= String.Format("Connect Timeout = {0};", devTimeout)

            'SQL Serverへ接続する
            cn.ConnectionString = connectionString
            cn.Open()

            'SQLを文字列として作る
            Dim SQL As String = ""
            SQL &= String.Format("SELECT COUNT(*) AS COUNT ")
            SQL &= String.Format("FROM UserInfo  ")
            SQL &= String.Format("WHERE user_id = '{0}' ", userID)
            SQL &= String.Format("AND ")
            SQL &= String.Format("password = '{0}' ", password)

            'SQLを実行する
            Dim cd As New SqlClient.SqlCommand
            cd.CommandText = SQL
            cd.Connection = cn

            'SQLの実行結果を取得する
            Dim dr As SqlClient.SqlDataReader
            dr = cd.ExecuteReader
            Dim count As Integer
            While dr.Read
                count = dr("COUNT")
            End While

            '取得結果を元に、認証結果を判断する。
            If count = 1 Then
                isAuthenticated = True
            Else
                isAuthenticated = False
            End If

        Catch ex As Exception
            systemErrorFlag = True
            MessageBox.Show("エラーが発生しました： " & ex.Message)
        Finally
            cn.Close()
            cn.Dispose()
        End Try

        Return systemErrorFlag
    End Function

End Class

問題点① このコードではSQLインジェクションが可能となってしまう

SQL文を単にコードに並べただけでは、SQLインジェクションが可能となってしまいます。（SQLインジェクションとはなんぞや！と言う人は、Google検索して、先に理解しておいた方が良いかも）

試しに、ユーザー情報を以下のように準備し、SQLインジェクションを試してみます。
※本来、パスワードをハッシュ化して管理したり、パスワードのテキストフォームは見えないように配慮しなければなりませんが、この時点では実装していません。

ユーザーIDとパスワードを入力し、ログインすると、認証の成功の有無が分かるようにしています。

ユーザーIDとパスワードが一致すれば、認証に成功というメッセージを出力するようにしています。

またパスワードを誤ったものにすると、ログインに失敗します。ここまでは想定している動作が行われていると思います。

しかしユーザーIDの後ろに、[' --]を追加すると、ユーザーIDのみで認証が成功してしまいます。

組み立てられるSQLは以下のようになっています。"--"の部分は、SQL Server上でコメントアウトという扱いになってしまいます。そのため、本来、ユーザーIDとパスワードに一致することを条件に認証成功としていますが、ユーザーIDのみの条件となってしまいます。

実行されるSQL

SELECT COUNT(*) AS COUNT FROM UserInfo  WHERE user_id = 'U1234567' --' AND password = '' 

結果、（どのようなアプリかにもよりますが、）これによって、"U1234567"ユーザーの個人情報が不正に閲覧・入手されるでしょう。

また、でたらめなユーザーIDの後ろに、[' or 1 = 1 --']を追加すると、ユーザーIDとパスワードに関係なく、認証成功となってしまいます。

実行されるSQL

SELECT COUNT(*) AS COUNT FROM UserInfo  WHERE user_id = 'U7654321' or 1 = 1 --'' AND password = '76543210' 

結果、（どのようなアプリかにもよりますが、）これによって、情報が不正に閲覧・入手されるでしょう。

他にも、SQL Server上に保存しているデータを削除することも可能になってしまいます。ユーザーIDの後ろに['; DELETE FROM UserInfo; --]を追加すると、ユーザー情報も削除できてしまいます。（今回は、削除の権限を付与していなかったため、ユーザー情報を削除される心配はありませんでした）

実行されるSQL

SELECT COUNT(*) AS COUNT FROM UserInfo  WHERE user_id = 'U'; DELETE FROM UserInfo; --' AND password = '01234567' 

修正した結果はこちら。

問題点②　取得したデータの数をカウントして、認証の有無を判断してしまっている

ユーザーIDとパスワードに一致するレコードの数を、SQL Server側でカウントし、それをアプリケーション上でカウント数をもとに条件分岐するというコードになっており、効率が悪くなっています。

SQLのコード修正が必要となった際、影響を受ける範囲が大きいことが保守的に問題となることもあります。また"count = 1"と書かれており、マジックナンバーとしてコードが書かれているので、別の開発者が見たときに、この"1"はどのような意味を持つのかが分からなくなるでしょう。

修正した結果はこちら。

注意事項

• この記事では、SQL Server、SSMS、visual studioを使用しているので、試してみたい方は事前に準備が必要です。
• 本記事に掲載されているコードは掲載用にサンプルとして掲載しているものです。したがって本記事を参考にし、コーディングを進める場合は作成しているアプリに応じて、各自で判断して、適応してください。
• 関連する記事の一覧はこちら。
  ※参照先のリンクが切れている場合は、トップページから確認してください。

この記事は誰向けの記事か？

• SQLインジェクション対策をコードに実装したい人
• SQLインジェクションを実際に試した場合のイメージをつかみたい人

環境

• OS：Windows10
• SQL Server
• SQL Server Management Studio（SSMS）:v19.0.2
• visual studio 2022

本記事における注意事項

• 本記事は、備忘録としてまとめたものになります。
• 他の方の参考になる可能性も踏まえて、一般公開も行なっております。
• また記載内容はすべて、正しい内容が記載されているとは限りません。
• 誤った内容を見つけた場合は、ご指摘をお願いいたします。