Conversation
899ba0d to
1f9a299
Compare
|
This seems real to me. Will reproduce after dinner. |
1f9a299 to
d33a36c
Compare
|
This was confirmed on my side using a test password such as: Merlin:Test123! So I was able to independently replicate this. Same in reverse, I type in: Merlin:Test123! in the WebUI, and it only displays : Merlin Considering this has been your journey since the start: I'll let you handle this one and assess if it needs any changes before merging ;) |
d33a36c to
b07a036
Compare
|
Thanks for reproducing the colon issue. I went through the credential/password history you linked, especially #159, #192, #561, #569, #587, #590 and #602. That history is actually why I followed the credential path end-to-end instead of stopping at the While tracing that path I noticed the second issue addressed here: the reusable Base64 credential was being kept in I have now rebased the PR onto the current Commit tested: The live test covered migration of the existing credential, mode 600 private storage, repeated startup, real router login, backend save without resending the password, and a real WebUI Apply with the password field left blank. After the test, The implementation is therefore no longer only statically reviewed on my side - the migration and normal WebUI/backend flow have now been exercised on actual hardware. @Martinski4GitHub, given the history of this credential path, I'm very interested in anything you would change in the approach before this is merged. |
|
I've read the PR description twice, but it's not clear to me what exactly the issue is that this PR is trying to address by creating a separate "protected" file for the router WebUI login credentials. Can you elaborate on the exact scenario or use case that this PR is trying to provide a solution for? If you can, show an example with context and details, and maybe some screenshots illustrating the problem might help. BTW, the bug in the WebUI regarding the colon separator between username & password was a great catch!! Thanks. |
|
@Martinski4GitHub thanks! I think I made the PR description too focused on the implementation and not enough on the concrete scenario. The separate file is not intended to protect the credentials from someone who already has root/SSH access to the router. That is not the threat model here. The issue I was trying to address is that, on current More specifically:
ln -sf "$CONFIG_FILE" "${SCRIPT_WEB_DIR}/config.htm"
url: '/ext/MerlinAU/config.htm'
case keyUpper === 'CREDENTIALS_BASE64':
var decoded = atob(keyValue);
var password = decoded.split(':')[1] || '';
ajax_custom_settings.routerPassword = password;
As a concrete example, if the router credentials were: the existing configuration would persist a reusable value equivalent to: Base64 itself is not the problem here. MerlinAU needs that representation for the ASUS login flow, and this PR keeps using it. The concern is that the saved reusable credential does not need to be sent back to the browser every time the MerlinAU WebUI is opened. The normal use case is:
For normal configuration display, the browser only really needs to know whether a credential is already stored, not what that credential is. That is what the separate file is intended to change. With this PR:
So I would describe the goal less as protecting the credential from the router administrator and more as keeping a reusable secret server-side instead of unnecessarily returning it through the WebUI configuration path. I used a separate file because If you think the credential does not need to be separated this way, or there is a cleaner approach that fits MerlinAU's existing settings architecture better, I'm absolutely open to changing it. The main thing I wanted to avoid was sending the already-stored router password back to the browser every time the configuration page is loaded. I’ll admit I’m a bit security-centric by nature, so my brain tends to wander through all sorts of possible edge cases and "what if" scenarios... even the slightly paranoid ones. 😅 And thanks regarding the colon bug! That was actually what led me to trace this whole credential path in the first place. 🙂 |
Why not? What is the perceived "threat" you're trying to address?
There is intention and purpose in presenting the current password string to the user (obfuscated by default every time you open the WebUI page to prevent "over-the-shoulder" exposure to someone else, same thing done in the SSH CLI menu). I can think of a couple of scenarios where making the actual password available to the user in the WebUI would be useful:
There may be other scenarios, but the point is that users can find it useful to have the password available at their fingertips on the WebUI. As you already know, if a hacker compromises the router via an SSH login, there's no point in protecting any files since the hacker has full access to the entire filesystem with root privileges. Or if the hacker has compromised the WebUI because of access over the WAN, they already have the login credentials anyway. At this point, I don't see any value in "protecting" the password by not making it available in the WebUI. Unless you have other reasons for your implementation, or explain exactly what the perceived threat is, I'm going to close this PR by Friday evening. I appreciate the effort, but there must be a clear and verifiable reason, purpose, or value in order to make such wide-ranging changes in the code. |
|
@Martinski4GitHub thanks for the detailed explanation. I understand the intended behavior much better now, particularly that making the existing password available in the WebUI is a deliberate UX choice rather than an accidental consequence of the current implementation. I still personally lean toward keeping reusable secrets server-side whenever the client does not strictly need them, so I think the approach in this PR can be defensible under a different threat model. But I also understand the usability cases you described and, ultimately, this is an architectural decision for MerlinAU and its maintainers to make. There is no point in me pushing the protected-storage part further when the current behavior is intentional and you both prefer to retain it. I also saw #614 and noticed that the colon parsing fix has already been incorporated there, thank you for that. That was the reproducible bug that originally started me down this rabbit hole, so I'm glad that part will make it into the project. From my side, then, there is nothing further that needs to be addressed in #613. Feel free to close it. Thanks to both of you for taking the time to review, test and discuss the proposal! |
Thanks @maghuro Thank you for identifying this issue for us though :) The password is protected by the file system, they would already need to know it to gain access to it via SSH or the WebUI (if exposed) Only exclusion would be some exploit allowing login without a password on ASUS side, but in vulnerability case that extreme, the hacker wouldn't need or be looking for a password at that stage anyways. |
Summary
This supersedes #612 and keeps its first-colon password parsing fix while also moving stored router credentials out of the WebUI-readable MerlinAU configuration.
The current credential format is Base64 of
username:password, which is required by the ASUS login flow. Base64 is therefore retained as the protocol representation, but the persistent value is now stored server-side in a protected file instead of being exposed throughconfig.htm.Changes
:, including passwords containing one or multiple colons./jffs/addons/MerlinAU.d/login_credentials.b64with mode600.credentials_base64 TBDin the WebUI-readable configuration for compatibility.credentials_stored ENABLED/DISABLEDstate for the WebUI.Validation
Tested on an ASUS GT-AX6000 running Asuswrt-Merlin.
Validated:
credentials_base64value;600;credentials_base64 TBD;MerlinAU_credentials_base64remains in shared settings;:, multiple colons, leading colons and trailing colons preserve the complete password after the first delimiter;sh -n.The branch contains two commits on top of current upstream
dev: the original #612 fix plus the protected-storage change.