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

OTA Unsigned binary after first one is signed, espota.py reports [INFO]: Result: OK but update fails on esp8266 #7162

Closed
maribeiro opened this issue Mar 21, 2020 · 4 comments · Fixed by #7204
Assignees

Comments

@maribeiro
Copy link

I have signed a binary and uploaded via OTA - so next OTA updates should be signed.
Then tried to see if the esp8266 would accept an unsigned binary - it's expected that it should not accept.
So I uploaded via OTA an unsigned binary, and espota.py reported [INFO]: Result: OK
But it actually failed on the esp8266 side with Error[4]: End Failed
First: why is espota reporting OK if it failed?
And second, shouldn't the error on the esp side be UPDATE_ERROR_SIGN ?

  • Hardware: ESP-12e
  • Core Version: latest git hash
  • Development Env: Arduino IDE && Platformio
  • Operating System: Windows

I have used ArduinoOTA sketch for this tests.

@devyte
Copy link
Collaborator

devyte commented Apr 4, 2020

@earlephilhower I seem to remember this was a choice to allow rollback. Is that correct?

@earlephilhower
Copy link
Collaborator

earlephilhower commented Apr 9, 2020

After digging through stuff, this is all in the ArduinoOTA, the BasicOTA sketch, and espota.py from what I can see and not a security risk as the Updater.cpp class is always rejecting invalid/unsigned bins.

In reverse order:

And second, shouldn't the error on the esp side be UPDATE_ERROR_SIGN ?

Updater (the guy who actually does the work) is returning UPDATE_ERROR_SIGN just fine. However, the ArduinoOTA class (wrapper which feeds Updater) doesn't define anything like SIGN_ERR and just returns ERROR_END when there's an error when calling Updater::end.

If we add a new error code from ArduinoOTA then existing apps may break/output incorrect error messages when a signing fails. Leaving that as-is, you get "Error End" which is correct, if not specific. @devyte , @d-a-v , thoughts?

First: why is espota reporting OK if it failed?

Arduino/tools/espota.py

Lines 190 to 195 in 41d271d

connection.settimeout(60)
while not received_ok:
if connection.recv(32).decode().find('O') >= 0:
# connection will receive only digits or 'OK'
received_ok = True;
logging.info('Result: OK')

Guess what happens when an OTA error happens...it sends "ERROR xxxx". That's got an O, so as far as espota cares it's "OK". D'oh!
edit: It's actually better than that. The received_ok var is set to True on every successful 1.4K chunk written to the ESP8266, but it is never reset to False. So the entire while loop is completely skipped and it jumps straight to the logging ok output!

I'll throw together a PR for the espota.py thing. The other bit, I think should leave as-is.

earlephilhower added a commit to earlephilhower/Arduino that referenced this issue Apr 9, 2020
The OTA script was not reporting the actual reported upload status from
the ESP8266, and instead always printed "Result: OK" no matter what
happened.

Now check for ERROR or OK in final message (and ensure the message is
not accidentally merged with the final byte count) and report properly.

Fixes esp8266#7162
@devyte
Copy link
Collaborator

devyte commented Apr 10, 2020

Agree with @earlephilhower. The behavior could be improved for the specific sign error, but if done it would have to be for next major.

@maribeiro
Copy link
Author

Thank you very much for your time.
It's not an urgent thing to do , so take your time :)

earlephilhower added a commit that referenced this issue Apr 16, 2020
The OTA script was not reporting the actual reported upload status from
the ESP8266, and instead always printed "Result: OK" no matter what
happened.

Now check for ERROR or OK in final message (and ensure the message is
not accidentally merged with the final byte count) and report properly.

Fixes #7162
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants