1

I have used map in my android application. I passed origin and destination latlon and get data from map url then parse the response.

But while auditing below code as marked for DOS attack stating that "This code might allow an attacker to crash the program or otherwise make it unavailable to legitimate users." Concern : What if attacker push too large file then it will go on line by line and loop will be run for too long. Proposed solution : Do not allow to read more than specific file size, so that it won't read file beyond some limit

Here is my code :

    String url = "https://maps.googleapis.com/maps/api/directions/json"+ "?" + str_origin + "&" + str_dest + "&" +  "sensor=false";

  private String downloadDataFromUrl(String strUrl) throws IOException {
            String data = "";
            InputStream iStream = null;
            HttpsURLConnection urlConnection = null;
            try {
                URL url = new URL(strUrl);
                urlConnection = (HttpsURLConnection) url.openConnection();
                urlConnection.connect();
                iStream = urlConnection.getInputStream();
                BufferedReader br = new BufferedReader(new InputStreamReader(iStream),1024);    
                StringBuffer sb = new StringBuffer();
                String line = "";
                while ((line = br.readLine()) != null) {
                    sb.append(line);
                }

                data = sb.toString();

                br.close();

            } catch (Exception e) {
                Log.d("Exception", e.toString());
            } finally {
                iStream.close();
                urlConnection.disconnect();
            }
            return data;
        }

Please provide solution. Thanks in advance.

Edit 1:by calling append() it appends Untrusted data to a StringBuilder instance initialized with the default backing-array size (16). This can cause the JVM to over-consume heap memory space.

Ross Ridge
  • 38,414
  • 7
  • 81
  • 112
jil123
  • 99
  • 1
  • 12
  • Is there some problem with not waiting for the readLine method to return null and checking bytes read instead? – comodoro Jul 19 '18 at 07:58
  • No. My auditors said its a bad practice to read file without checking its size. If attacker had push 1GB file then it may be enter in some never ending loop. – jil123 Jul 19 '18 at 08:29
  • @comodoro please check edit part – jil123 Jul 19 '18 at 08:33

1 Answers1

1

If you download from an unknown URL, the data can indeed be arbitrary and BufferedReader.readLine() can encounter a line so long the program cannot handle it. This question indicates that limiting BufferedReader line length may not be trivial.

Number of lines can be too big as well, in which case line count check instead of simple null check in the while loop seems to be enough.

Question is why would you allow the user to input an arbitrary URL and download it without checking. The URL can easily be a several GB binary file. Your first line indicates that you intend to use the Google Maps API, which AFAIK does not return excessively large lines, rendering the DOS concern moot (except in some ultrasecure applications, which I do not think Android is suitable to use for).

Community
  • 1
  • 1
comodoro
  • 1,489
  • 1
  • 19
  • 30