e-m-b-a / emba

EMBA - The firmware security analyzer
https://www.securefirmware.de
GNU General Public License v3.0
2.49k stars 223 forks source link

restart EMBA functionality #1078

Closed m-1-k-3 closed 4 months ago

m-1-k-3 commented 4 months ago

Bug fix

see https://github.com/e-m-b-a/emba/issues/1077 and https://github.com/e-m-b-a/emba/discussions/1073

the original "buggy" restart functionality should be recovered closes https://github.com/e-m-b-a/emba/issues/1077

image

CowBoy4mH3LL commented 4 months ago

Ok so that did not completely solve the problem.

If we consider the profile full and final and something was terminated, the string "Test ended" would not be there and check_emba_ended() would not return true and restart ensues.

But, if the analyst incrementally changes the profile, we have a problem. For example, in my profile I had everything black listed except the P02 -- cause I wanted to just see the initial analysis and make my choices after that. In this case, the analysis obviously finished successfully and "Test ended" was logged at the end of the log file

[!] Wed Mar  6 23:49:49 EST 2024 - Test ended on Wed Mar  6 23:49:49 EST 2024 and took about 0 days and 00:00:36 

Next time, when I enabled the corresponding extraction/unpacking module etc. the restart would fail. As such, the differential restart requirement was raised in #1073. That is, at this point, if I replied to using "n" at the log deletion prompt EMBA would terminate because check_emba_ended() would not correctly indicate that emba has NOT_FINISHED in the log_folder() function.

CowBoy4mH3LL commented 4 months ago

Currently the way I am handling this is by exporting a KEEP_GOING=1 variable in my profile file and making two changes

  1. in check_emba_ended() I add a check at the beginning of the function. This ensures the NOT_FINISHED env is set to 1 at line 38 in log_folder()
    check_emba_ended() {
    if [ $KEEP_GOING -eq 1 ]; then
    return 1
    fi
    if grep -q "Test ended" "${LOG_DIR}""/""${MAIN_LOG_FILE}"; then
    # EMBA is already finished
    return 0
    fi
    return 1
    }
  2. In log_folder(), I added a patch at line 69 that ensures the KEEP_GOING variable is respected
    if [[ ${OVERWRITE_LOG} -eq 1 ]] ; then
      ANSWER="y"
    elif [[ ${KEEP_GOING} -eq 1 ]] ; then
      ANSWER="n" 
    else
      read -p "(Y/n)  " -r ANSWER
    fi

Now it works.

CowBoy4mH3LL commented 4 months ago

But ideally we should the following

  1. If the firmware digest is different -- terminate and ask for a new log path
  2. If they are the same, keep going without prompting for deletion at all because there is already a switch (-y) to do that. Instead, act as if this is a restart.
m-1-k-3 commented 4 months ago

If we consider the profile full and final and something was terminated, the string "Test ended" would not be there and check_emba_ended() would not return true and restart ensues.

fine, this is the idea of this functionality.

But, if the analyst incrementally changes the profile, we have a problem. For example, in my profile I had everything black listed except the P02 -- cause I wanted to just see the initial analysis and make my choices after that. In this case, the analysis obviously finished successfully and "Test ended" was logged at the end of the log file

You could remove the "Test ended" entry in the log file via sed or so.

[!] Wed Mar  6 23:49:49 EST 2024 - Test ended on Wed Mar  6 23:49:49 EST 2024 and took about 0 days and 00:00:36 

Next time, when I enabled the corresponding extraction/unpacking module etc. the restart would fail. As such, the differential restart requirement was raised in #1073. That is, at this point, if I replied to using "n" at the log deletion prompt EMBA would terminate because check_emba_ended() would not correctly indicate that emba has NOT_FINISHED in the log_folder() function.

Yes, if the test is ended then the current functionality does not cover your use case and you need to adjust emba.log or log to a new log directory or overwrite the current log.

m-1-k-3 commented 4 months ago

But ideally we should the following

  1. If the firmware digest is different -- terminate and ask for a new log path
  2. If they are the same, keep going without prompting for deletion at all because there is already a switch (-y) to do that. Instead, act as if this is a restart.

ad 2 - As the complete rescan feature is not that well tested I would not do it as default but we can introduce your approach you have shown in here https://github.com/e-m-b-a/emba/pull/1078#issuecomment-1982479105 With this in place a user can set an environment variable and then the rescan will catch in, otherwise everything stays the same. Could you do a PR for this?