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.

Sign up for GitHub

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

Jump to bottom

Bug in parsing file upload through http form #7542

Closed
mjforan opened this issue Aug 23, 2020 · 1 comment · Fixed by #7543
Closed

Bug in parsing file upload through http form #7542

mjforan opened this issue Aug 23, 2020 · 1 comment · Fixed by #7543

Comments

@mjforan
Copy link
Contributor

mjforan commented Aug 23, 2020

Platform

  • Hardware: ESP8285 / ESP8266
  • Core Version: 2.7.4 (Package installed from Arduino IDE board manager)
  • Development Env: Arduino IDE
  • Operating System: Windows

Settings in IDE

  • Module: Generic ESP8266 Module
  • Flash Mode: dout
  • Flash Size: 1MB
  • lwip Variant: v2 Lower Memory
  • Reset Method: nodemcu
  • Flash Frequency: 40Mhz
  • CPU Frequency: 80Mhz
  • Upload Using: SERIAL
  • Upload Speed: 3000000

Problem Description

The scenario is uploading a file through an HTML form. This uses a POST request to send multipart form data, although the files I'm using are small enough that they fit within one request buffer (i.e. <2048 bytes). The small program below accepts the file from the form and prints out the buffer contents. This usually works. For my application I am trying to upload new root certificates in X.509 format. This causes a problem because the file contains "-----END CERTIFICATE-----" at the end of the file. In multipart form data, the separator for fields is a line called "boundary," which starts with (at least) two dashes. For example, I'm using FireFox and each form field is separated by some unique string like "-----------------------------26263539015755059383526350366" Here is a link to the relevant section of the library. There is a bug in the logic checking if it is actually the boundary line. In my example I included an extra field below the file entry. When I print the file buffer, it includes the boundary string and the data of the next form entry. If I don't include the extra form entry, the upload hangs while the server waits for the end of the entry.

What is going wrong in the algorithm: You notice the current line starts with two dashes and decide to check if it is the boundary line. You read in enough characters to check if it is the boundary string. However, the actual line is shorter than the boundary string, so you end up reading in the current line plus half of the next one (the actual boundary string). Then you see it does not match the boundary string, so you go back to reading the file. Again you notice two dashes, and read in the length of the boundary string. Since you were already halfway through the line containing the boundary, the stuff you read in doesn't fully match the boundary. So you continue on reading into the next entry until you hit the second entry's boundary, which is handled correctly.

How to fix it: Instead of reading the length of the boundary string, make sure to only read to the end of the current line.

Example Sketch

Here is the file I used for testing: DST_Root_CA_X3.pem
Don't forget to fill in your own WiFi credentials

#include <ESP8266WiFi.h>
#include <WiFiClient.h>
#include <ESP8266WebServer.h>

ESP8266WebServer server(80);

const String htmlDoc = "<html>\
  <head>\
    <meta charset=\"UTF-8\">\
  </head>\
  <body>\
    <form method=\"post\" enctype=\"multipart/form-data\" action=\"/index.html\">\
      <label for=\"caCert\">CA root certificate:</label><br><input type=\"file\" id=\"caCert\" name=\"caCert\"><br>\
      <label for=\"ssid\">SSID:</label><input type=\"text\" id=\"ssid\" name=\"ssid\"><br>\
      <input type=\"submit\" value=\"Submit\">\
    </form>\
  </body>\
</html>";


void setup(void) {
  Serial.begin(115200);
  WiFi.begin("your-ssid", "your-password");
  while (WiFi.status() != WL_CONNECTED)
    delay(500);
  Serial.print("\n\rIP address: ");
  Serial.println(WiFi.localIP());

  server.onFileUpload([]() { 
    if(server.upload().status == UPLOAD_FILE_WRITE)
      Serial.println(String((const char*)server.upload().buf)); 
  });
  
  server.on("/index.html", [](){server.send(200, "text/html", htmlDoc);});
  server.begin();
}

void loop(void) {
  server.handleClient();
}

Debug Messages

New client
request: POST /index.html HTTP/1.1
method: POST url: /index.html search: 
headerName: Host
headerValue: 192.168.9.114
headerName: User-Agent
headerValue: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:79.0) Gecko/20100101 Firefox/79.0
headerName: Accept
headerValue: text/html,application/xhtml+xml,application/xml;q=0.9,image/webp,*/*;q=0.8
headerName: Accept-Language
headerValue: en-US,en;q=0.5
headerName: Accept-Encoding
headerValue: gzip, deflate
headerName: Content-Type
headerValue: multipart/form-data; boundary=---------------------------32093566031658169028293882063
headerName: Content-Length
headerValue: 1573
headerName: Origin
headerValue: http://192.168.9.114
headerName: Connection
headerValue: keep-alive
headerName: Referer
headerValue: http://192.168.9.114/index.html
headerName: Upgrade-Insecure-Requests
headerValue: 1
args: 
args count: 0
args: 
args count: 0
Parse Form: Boundary: ---------------------------32093566031658169028293882063 Length: 1573
PostArg FileName: DST_Root_CA_X3.pem
PostArg Name: caCert
PostArg Type: application/octet-stream
Start File: DST_Root_CA_X3.pem Type: application/octet-stream
-----BEGIN CERTIFICATE-----
MIIDSjCCAjKgAwIBAgIQRK+wgNajJ7qJMDmGLvhAazANBgkqhkiG9w0BAQUFADA/
MSQwIgYDVQQKExtEaWdpdGFsIFNpZ25hdHVyZSBUcnVzdCBDby4xFzAVBgNVBAMT
DkRTVCBSb290IENBIFgzMB4XDTAwMDkzMDIxMTIxOVoXDTIxMDkzMDE0MDExNVow
PzEkMCIGA1UEChMbRGlnaXRhbCBTaWduYXR1cmUgVHJ1c3QgQ28uMRcwFQYDVQQD
Ew5EU1QgUm9vdCBDQSBYMzCCASIwDQYJKoZIhvcNAQEBBQADggEPADCCAQoCggEB
AN+v6ZdQCINXtMxiZfaQguzH0yxrMMpb7NnDfcdAwRgUi+DoM3ZJKuM/IUmTrE4O
rz5Iy2Xu/NMhD2XSKtkyj4zl93ewEnu1lcCJo6m67XMuegwGMoOifooUMM0RoOEq
OLl5CjH9UL2AZd+3UWODyOKIYepLYYHsUmu5ouJLGiifSKOeDNoJjj4XLh7dIN9b
xiqKqy69cK3FCxolkHRyxXtqqzTWMIn/5WgTe1QLyNau7Fqckh49ZLOMxt+/yUFw
7BZy1SbsOFU5Q9D8/RhcQPGX69Wam40dutolucbY38EVAjqr2m7xPi71XAicPNaD
aeQQmxkqtilX4+U9m5/wAl0CAwEAAaNCMEAwDwYDVR0TAQH/BAUwAwEB/zAOBgNV
HQ8BAf8EBAMCAQYwHQYDVR0OBBYEFMSnsaR7LHH62+FLkHX/xBVghYkQMA0GCSqG
SIb3DQEBBQUAA4IBAQCjGiybFwBcqR7uKGY3Or+Dxz9LwwmglSBd49lZRNI+DT69
ikugdB/OEIKcdBodfpga3csTS7MgROSR6cz8faXbauX+5v3gTt23ADq1cEmv8uXr
AvHRAosZy5Q6XkjEGB5YGV8eAlrwDPGxrancWYaLbumR9YbK+rlmM6pZW87ipxZz
R8srzJmwN0jP41ZL9c8PDHIyh8bwRLtTcm1D9SZImlJnt1ir/md2cXjbDaJWFBM5
JDGFoqgCWjBH4d1QB7wCCZAA62RjYJsWvIjJEubSfZGL+T0yjWW06XyxV3bqxbYo
Ob8VZRzI9neWagqNdwvYkQsEjgfbKbYK7p2CNTUQ
-----END CERTIFICATE-----

-----------------------------32093566031658169028293882063
Content-Disposition: form-data; name="ssid"


End File: DST_Root_CA_X3.pem Type: application/octet-stream Size: 1329
Done Parsing POST
Request: /index.html
Arguments: 
final list of key/value pairs:
pm open,type:2 0
@earlephilhower
Copy link
Collaborator

Good debug, @mjforanVT . Why not submit a PR since you've done pretty much all the work already?

@mjforan mjforan mentioned this issue Aug 23, 2020
Merged
earlephilhower pushed a commit that referenced this issue Nov 2, 2020
@mjforan
Fixed bug in parsing POST file uploads (#7543)
5d2563e 
The boundary parsing in the webserver could end up missing boundaries if the
uploaded file had `--` at the start of the line because it read in the entire boundary
length worth of bytes.  Fix by only reading up to either the boundary length or
a newline, avoiding the issue.

Fixes #7542
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

fixed bug in parsing POST file uploads
2 participants
@earlephilhower @mjforan