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

treewide: rpcd scripts needs to be executable in git index #7526

Open
1 task done
dannil opened this issue Jan 4, 2025 · 6 comments
Open
1 task done

treewide: rpcd scripts needs to be executable in git index #7526

dannil opened this issue Jan 4, 2025 · 6 comments

Comments

@dannil
Copy link
Contributor

dannil commented Jan 4, 2025

Is there an existing issue for this?

  • I have searched among all existing issues (including closed issues)

screenshots or captures

Tagging some of the currently most active developers in the repo for discussion.

@systemcrash @stokito @Ramon00 @rdmitry0911 @stangri

Spawned from https://forum.openwrt.org/t/openwrt-24-10-0-rc4-fourth-release-candidate/219368/142?u=dannil

It seems that all apps which doesn't have it's respective RPC scripts set with the executable mask, which at the moment is luci-app-olsr-viz and luci-app-squid, isn't able to register with ubus and therefore crashes in the UI with the very non-descriptive error RPC call to <name of RPC object> failed with error -32000: Object not found

Screenshot_24
Screenshot_25

This makes sense cause if you invoke the RPC script directly on the command line it also fails

root@labmouse:~# /usr/libexec/rpcd/luci.squid
-ash: /usr/libexec/rpcd/luci.squid: Permission denied

If you then invoke chmod +x /usr/libexec/rpcd/luci.squid followed by service rpcd restart, the Object not found error will be gone.

The affected apps were found with the following command

~/git/openwrt_parent/luci master > find . -type d -name rpcd -exec sh -c 'ls -la "$1"' _ {} \; | grep -v '^d' | grep -v '^total'
-rwxr-xr-x 1 vm vm  713 Jun 25  2024 luci.yggdrasil
-rwxr-xr-x 1 vm vm  791 Jun 25  2024 luci.yggdrasil-jumper
-rwxr-xr-x 1 vm vm  897 Jun 25  2024 luci.battstatus
-rwxr-xr-x 1 vm vm 5016 Jun 25  2024 luci.https-dns-proxy
-rwxr-xr-x 1 vm vm 8501 Jun 25  2024 luci.example
-rwxr-xr-x 1 vm vm 8750 Jan  4 22:45 luci.adblock-fast
-rwxr-xr-x 1 vm vm 9317 Nov 17 11:32 luci.pbr
-rwxr-xr-x 1 vm vm 12577 Jun 25  2024 luci.advanced_reboot
-rwxr-xr-x 1 vm vm 1107 Jun 25  2024 olsr-services
-rwxr-xr-x 1 vm vm 1561 Jun 25  2024 olsrinfo
-rw-r--r-- 1 vm vm  699 Jan  4 23:14 luci.squid
-rw-r--r-- 1 vm vm 1378 Jan  4 22:45 olsrvizinfo

I have a hunch that this is one of the causes for some of the historical issues when users have been faced with unsuspected Object not found errors even when the RPC script file is located in the correct directory.

Also, this may be hard to discover during development since you may have inconsistent file masks between the SCP'd/rsync'ed file on your development environment versus the git index. I'm thinking mainly of Windows here where this is a big issue since you can't even set umasks there unless in Git Bash/WSL or using git update-index. In some cases the umask isn't even preserved when you copy it to your development environment depending on parameters/tool (for exampIe, I don't think SCP preserves it unless you pass the -p option), then anything goes.

This is an invitation to discuss how to handle this going forward, because it'll definitely happen again. I personally would've expected the postinst step to automatically handle this but obviously that isn't how it works.

Actual behaviour

Some apps with RPC scripts crashes/throws warnings.

Expected behaviour

All apps with RPC scripts should be usable.

Steps to reproduce

  1. Install any app which doesn't specify the rpcd scripts as executable
  2. Hit the code path doing the RPC code invocation
  3. The app crashes/throws warnings

Additional Information

{
        "kernel": "6.6.67",
        "hostname": "labmouse",
        "system": "AMD Ryzen 7 5700X3D 8-Core Processor",
        "model": "VMware, Inc. VMware Virtual Platform",
        "board_name": "vmware-inc-vmware-virtual-platform",
        "rootfs_type": "ext4",
        "release": {
                "distribution": "OpenWrt",
                "version": "24.10.0-rc4",
                "revision": "r28211-d55754ce0d",
                "target": "x86/64",
                "description": "OpenWrt 24.10.0-rc4 r28211-d55754ce0d",
                "builddate": "1734915335"
        }
}

What browsers do you see the problem on?

Firefox, Chrome, Safari, Microsoft Edge

Relevant log output

No response

@dannil
Copy link
Contributor Author

dannil commented Jan 4, 2025

I've compiled ipk's for luci-app-squid with both -x and +x and as expected, if you install the executable ipk as attached here the app now works correctly, I've attached the ZIP of both of 'em here cause GitHub doesn't like the .ipk extension.

luci-app-squid_24.362.67717~235ec08_all.zip

dannil added a commit to dannil/luci that referenced this issue Jan 4, 2025
The git index needs the executable permission to be set to correctly register it with ubus.

Link: openwrt#7526

Signed-off-by: Daniel Nilsson <[email protected]>
@dannil dannil changed the title treewide: rpcd scripts needs to be executable in git index? treewide: rpcd scripts needs to be executable in git index Jan 4, 2025
dannil added a commit to dannil/luci that referenced this issue Jan 5, 2025
The git index needs the executable permission on RPC scripts to correctly
register them with ubus.

Link: openwrt#7526

Signed-off-by: Daniel Nilsson <[email protected]>
@stokito
Copy link
Contributor

stokito commented Jan 5, 2025

Which options do we have?

  1. CI check for the x flag.
  2. Make the Luci Makefile to chmod +x.
  3. Make the rpcd to assign the x flag to all scripts inside of the /usr/libexec/rpcd/

The last would be great but violates a single responsibility principle. I think the 1 should be enough.

systemcrash pushed a commit that referenced this issue Jan 5, 2025
The git index needs the executable permission on RPC scripts to correctly
register them with ubus.

Link: #7526

Signed-off-by: Daniel Nilsson <[email protected]>
@systemcrash
Copy link
Contributor

1 would be good. Ensure that various folders which typically contain .uc or .sh have +x.

@dannil
Copy link
Contributor Author

dannil commented Jan 5, 2025

Which options do we have?

1. CI check for the x flag.

2. Make the Luci Makefile to `chmod +x`.

3. Make the rpcd to assign the x flag to all scripts inside of the `/usr/libexec/rpcd/`

The last would be great but violates a single responsibility principle. I think the 1 should be enough.

I also think option 1 would be a good and straightforward way if someone wants to try to implement it, then we can ensure consistency as early as possible. Option 2 could work as a alternative but then you could run into situations where a file is copied directly to the target system from the repo (for example when you do direct changes to a JS file via scp/rsync/sshfs or whatever) without going through the build system, which wouldn't solve the inconsistency problem as mentioned earlier.

I can add that this is actually documented in https://openwrt.org/docs/techref/rpcd#plugin_executables near the end of the example where it makes the file executable, so it's at least a known and documented behavior (which makes sense 'cause if the RPC script isn't set as executable how could ubus invoke it?) and we should try to check for it nonetheless.

@systemcrash
Copy link
Contributor

  1. is reasonable. ( a bit more work, although if there's a copy-paste example in the repo, that's a plus )
  2. is a possibly step too far - even though it's what a user may want, blindly chmod +xing files in a folder on a running system can have unintended consequences. Unlikely but possible.

@hnyman
Copy link
Contributor

hnyman commented Jan 8, 2025

3 sounds like a nicely prepared step in an attack vector. Good xz points for you.

2 looks like the simple solution that would cover the normal use.
Luci.mk already copies files by path, and is executed for each package.
Control would stay inside a single package.

Simplest would be to add a section to luci.mk for root/user/libexec/rpcd/ to copy files and use the INSTALL_BIN macro. If the section is right after the normal root section copy, the files would get copied twice, but the latter action would ensure chmod.

luci/luci.mk

Lines 216 to 219 in ab1bf13

ifneq ($(wildcard ${CURDIR}/root),)
$(INSTALL_DIR) $(1)/
cp -pR $(PKG_BUILD_DIR)/root/* $(1)/
endif

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

No branches or pull requests

4 participants